refactor: drop dependency on net_processing for multiple consensus related objects #6934
Conversation
…overedSigsToVerify and its callers
…nd ProcessMessageGetSigShares
…NV and QGETSIGSHARES
|
This pull request has conflicts, please rebase. |
|
@PastaPastaPasta @UdjinM6 @kwvg Please give a look to this PR; I am looking for a concept review; any ideas & improvements on architecture level is welcome; thought any nits, typos, minor improvements will be considered by not expected yet. If there's no objections or no major improvements, I am going to start from next week create smaller PRs which is merge-friendly (smaller scope to review; done self-review, all linters are happy, changes are atomic and separate). |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR introduces a significant architectural refactoring to decouple consensus/chain code from network/node code by introducing the NetHandler interface pattern. The changes successfully move network message processing logic from net_processing.cpp into specialized handlers (NetInstantSend, NetSigning, NetGovernance), reducing circular dependencies from 56 to 37.
Key improvements:
- New
NetHandlerbase class provides lifecycle management (Start/Stop/Interrupt/Schedule) PeerManagerInternalinterface allows handlers to interact with peer management without direct couplingMessageProcessingResultstruct replaces directPeerManagercalls in consensus code- Network-specific code separated from consensus logic in signing, InstantSend, and governance modules
- Governance sync moved from
masternode/sync.cppto newnet_governance.cpp
Architecture changes:
- Handlers registered via
PeerManager::AddExtraHandler()during initialization - Message processing loop delegates to handlers sequentially
- Each handler manages its own worker threads and scheduling
- Shutdown sequence properly interrupts and stops handlers before cleanup
Confidence Score: 3/5
- Architectural refactoring is sound but marked as draft/PoC with known threading concerns
- The PR achieves its stated goal of decoupling consensus from networking code and significantly reduces circular dependencies. However, several factors warrant careful review: (1) PR is marked as draft/proof-of-concept by author, (2) author expects thread safety analysis to fail, (3) commented-out destructor code suggests uncertainty about lifecycle management, (4) duplicate thread names in
NetSigninghamper debugging, (5) large refactoring touching 55 files with complex threading logic needs extensive testing - Focus testing on
src/net_processing.cpp,src/net_signing.cpp, andsrc/init.cppfor proper handler lifecycle and thread safety. Verify shutdown sequence under various conditions.
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/net_processing.cpp | 3/5 | Implemented handler management (Add/Remove/Start/Stop/Schedule) and PeerManagerInternal delegation methods. Destructor contains commented-out code. Message processing now delegates to handlers in loop. |
| src/net_instantsend.cpp | 4/5 | Implements ISDLOCK message processing with batch BLS signature verification. Thread lifecycle properly managed with interrupt mechanism. Moved from instantsend/instantsend.cpp. |
| src/net_signing.cpp | 3/5 | Implements recovered signature and sig share processing with batch verification. Two worker threads share same thread name "recsigs". Thread lifecycle uses assertions for validation. |
| src/net_governance.cpp | 4/5 | Moved governance sync logic from masternode/sync.cpp. Implements periodic governance object/vote requests and sync state management via scheduler. |
| src/init.cpp | 4/5 | Updated initialization sequence to register handlers after PeerManager creation. Shutdown sequence properly calls Interrupt/Remove/Stop handlers. Handler order matters for dependencies. |
| src/llmq/signing_shares.cpp | 3/5 | Large refactoring removed network message processing code moved to net_signing.cpp. Simplified interface reduces circular dependencies. |
Sequence Diagram
sequenceDiagram
participant Init as init.cpp
participant PM as PeerManager
participant NH as NetHandler
participant NIS as NetInstantSend
participant NS as NetSigning
participant NG as NetGovernance
participant ISM as InstantSendManager
participant SM as SigningManager
participant GM as GovernanceManager
Note over Init,GM: Initialization Phase
Init->>PM: Create PeerManager
Init->>PM: AddExtraHandler(NetInstantSend)
PM->>NIS: Create with ISManager & QManager
Init->>PM: AddExtraHandler(NetSigning)
PM->>NS: Create with SigningManager & SharesManager
Note over Init: Wait for govman.IsValid()
Init->>PM: AddExtraHandler(NetGovernance)
PM->>NG: Create with GovernanceManager
Note over Init,GM: Start Phase
Init->>PM: StartHandlers()
PM->>NIS: Start()
NIS->>NIS: Launch WorkThread
PM->>NS: Start()
NS->>NS: Launch signing_thread & shares_thread
Init->>PM: ScheduleHandlers(scheduler)
PM->>NG: Schedule(scheduler)
NG->>NG: Register periodic tasks
Note over Init,GM: Message Processing
PM->>PM: ProcessMessage(peer, msg_type)
loop For each handler
PM->>NH: ProcessMessage(peer, msg_type)
alt ISDLOCK message
NH->>NIS: ProcessMessage()
NIS->>ISM: EnqueueInstantSendLock()
else QSIGREC/QSIGSHARE message
NH->>NS: ProcessMessage()
NS->>SM: ProcessRecoveredSig()
end
end
Note over Init,GM: Shutdown Phase
Init->>PM: InterruptHandlers()
PM->>NIS: Interrupt()
NIS->>NIS: Set workInterrupt
PM->>NS: Interrupt()
NS->>NS: Set workInterrupt
Init->>PM: RemoveHandlers()
PM->>NIS: Stop()
NIS->>NIS: Join WorkThread
PM->>NS: Stop()
NS->>NS: Join signing_thread & shares_thread
PM->>PM: Clear m_handlers
Additional Comments (1)
-
src/net_signing.cpp, line 406-410 (link)style: both worker threads use identical thread name "recsigs", making debugging difficult
55 files reviewed, 2 comments
| ~PeerManagerImpl() { | ||
| // probably do some disassignment | ||
| // m_handlers.clear(); | ||
| } |
There was a problem hiding this comment.
style: commented-out code in destructor should be removed or uncommented
the NetHandler base class destructor already calls Interrupt() and Stop(), so handlers should be cleaned up automatically. Either remove this comment or clarify why explicit cleanup might be needed
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/net_processing.cpp
Line: 598:601
Comment:
**style:** commented-out code in destructor should be removed or uncommented
the `NetHandler` base class destructor already calls `Interrupt()` and `Stop()`, so handlers should be cleaned up automatically. Either remove this comment or clarify why explicit cleanup might be needed
How can I resolve this? If you propose a fix, please make it concise.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR performs a comprehensive refactoring of the node's synchronization, networking, and signing architecture. The primary changes include: (1) relocating and renaming the masternode sync system to the node layer with constants renamed from Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/node/chainstate.cpp (1)
243-259: Address TODO before merging: handler reconnection is incomplete.The TODO comment indicates that network handlers (NetGovernance, NetInstantSend, NetSigning) need to be reconnected after the LLMQContext is recreated. This is critical for the new NetHandler architecture to function correctly after chainstate resets or reindexing.
Since this is marked as a draft PR, would you like me to help design the handler reconnection logic? The handlers would need to be:
- Stored in a way that survives the LLMQContext reset
- Re-registered with the new PeerManager instance after line 258
For reference, the test setup code at src/test/util/setup_common.cpp:370-375 shows how these handlers are initially created.
src/net_processing.h (1)
9-16: Add util/check.h for Assert usage
NetHandlernow callsAssert(peer_manager)in the header, but this file doesn't include<util/check.h>. That symbol won't be declared here, so the build breaks as soon as the header is consumed. Please add the missing include.#include <net.h> #include <validationinterface.h> #include <version.h> +#include <util/check.h>
🧹 Nitpick comments (5)
src/governance/object.cpp (1)
12-13: OK to use <node/sync.h>; consider pruning if unusedIf this file no longer needs CMasternodeSync or related symbols, you can drop <node/sync.h> to reduce compile-time coupling. Optional.
src/coinjoin/server.cpp (1)
11-11: Include update is fine; net_processing dependency remains for future refactoring.This file still includes
net_processing.hand usesPeerManagerdirectly (member variable at line 39 in server.h, parameter at line 99). Given the PR objective to decouple consensus-like components from net_processing, consider migrating to the proposed NetHandler bridge in a follow-up.src/test/util/setup_common.cpp (1)
370-375: Fix typo in commented-out code before uncommenting.Line 374 has an inconsistency:
node.peermanshould bem_node.peermanto match the pattern used on line 373 and elsewhere in the file.Apply this diff to fix the typo:
- node.peerman->AddExtraHandler(std::make_unique<NetGovernance>(node.peerman.get(), *node.govman, *node.mn_sync, *node.netfulfilledman)); + m_node.peerman->AddExtraHandler(std::make_unique<NetGovernance>(m_node.peerman.get(), *m_node.govman, *m_node.mn_sync, *m_node.netfulfilledman));src/llmq/signing.h (1)
197-198: Line 197: MakeQman()const so read-only callers can use it.
Qman()returns a const reference but the method itself isn't const, which means code that only has aconst CSigningManager&can't access the quorum manager. Adding the const qualifier keeps the new accessor usable in those read-only contexts without changing behavior.src/llmq/signing.cpp (1)
496-500: Consider optimization: returning listeners by value.The method returns a copy of the listeners vector. While thread-safe, this could be optimized if performance becomes a concern:
- Consider returning
const std::vector<CRecoveredSigsListener*>&with the lock held externally- Or use a callback pattern to avoid copying
For now, correctness is prioritized over performance, which is appropriate for this refactoring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
src/Makefile.am(4 hunks)src/chainlock/chainlock.cpp(1 hunks)src/chainlock/signing.cpp(2 hunks)src/coinjoin/client.cpp(1 hunks)src/coinjoin/coinjoin.cpp(1 hunks)src/coinjoin/server.cpp(1 hunks)src/dsnotificationinterface.cpp(1 hunks)src/evo/mnauth.cpp(1 hunks)src/governance/governance.cpp(15 hunks)src/governance/governance.h(3 hunks)src/governance/object.cpp(1 hunks)src/governance/signing.cpp(1 hunks)src/governance/vote.cpp(1 hunks)src/init.cpp(7 hunks)src/instantsend/instantsend.cpp(8 hunks)src/instantsend/instantsend.h(5 hunks)src/instantsend/signing.cpp(3 hunks)src/llmq/context.cpp(1 hunks)src/llmq/ehf_signals.cpp(2 hunks)src/llmq/quorums.cpp(1 hunks)src/llmq/signing.cpp(8 hunks)src/llmq/signing.h(5 hunks)src/llmq/signing_shares.cpp(25 hunks)src/llmq/signing_shares.h(3 hunks)src/masternode/active/context.cpp(1 hunks)src/masternode/active/notificationinterface.cpp(0 hunks)src/masternode/active/notificationinterface.h(0 hunks)src/masternode/payments.cpp(1 hunks)src/masternode/sync.cpp(0 hunks)src/masternode/utils.cpp(1 hunks)src/msg_result.h(0 hunks)src/net.cpp(1 hunks)src/net.h(1 hunks)src/net_governance.cpp(1 hunks)src/net_governance.h(1 hunks)src/net_instantsend.cpp(1 hunks)src/net_instantsend.h(1 hunks)src/net_processing.cpp(7 hunks)src/net_processing.h(2 hunks)src/net_signing.cpp(1 hunks)src/net_signing.h(1 hunks)src/node/chainstate.cpp(2 hunks)src/node/context.cpp(1 hunks)src/node/interfaces.cpp(1 hunks)src/node/sync.cpp(1 hunks)src/node/sync.h(4 hunks)src/rpc/governance.cpp(1 hunks)src/rpc/mining.cpp(1 hunks)src/rpc/node.cpp(1 hunks)src/rpc/quorums.cpp(1 hunks)src/test/util/net.h(1 hunks)src/test/util/setup_common.cpp(3 hunks)src/validation.h(0 hunks)src/wallet/interfaces.cpp(1 hunks)test/lint/lint-circular-dependencies.py(2 hunks)
💤 Files with no reviewable changes (5)
- src/masternode/active/notificationinterface.h
- src/msg_result.h
- src/masternode/sync.cpp
- src/validation.h
- src/masternode/active/notificationinterface.cpp
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/governance/signing.cppsrc/rpc/governance.cppsrc/coinjoin/server.cppsrc/node/interfaces.cppsrc/test/util/setup_common.cppsrc/coinjoin/client.cppsrc/coinjoin/coinjoin.cppsrc/wallet/interfaces.cppsrc/masternode/payments.cppsrc/chainlock/chainlock.cppsrc/evo/mnauth.cppsrc/instantsend/signing.cppsrc/rpc/quorums.cppsrc/chainlock/signing.cppsrc/masternode/utils.cppsrc/net.cppsrc/rpc/node.cppsrc/rpc/mining.cppsrc/governance/object.cppsrc/node/sync.hsrc/llmq/context.cppsrc/net_instantsend.hsrc/llmq/quorums.cppsrc/dsnotificationinterface.cppsrc/node/chainstate.cppsrc/llmq/ehf_signals.cppsrc/masternode/active/context.cppsrc/net_processing.hsrc/init.cppsrc/net.hsrc/llmq/signing.hsrc/net_signing.hsrc/net_governance.hsrc/test/util/net.hsrc/net_governance.cppsrc/net_signing.cppsrc/node/context.cppsrc/instantsend/instantsend.hsrc/governance/vote.cppsrc/net_instantsend.cppsrc/llmq/signing_shares.hsrc/governance/governance.cppsrc/governance/governance.hsrc/instantsend/instantsend.cppsrc/node/sync.cppsrc/net_processing.cppsrc/llmq/signing.cppsrc/llmq/signing_shares.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/util/setup_common.cppsrc/test/util/net.h
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/payments.cppsrc/evo/mnauth.cppsrc/masternode/utils.cppsrc/masternode/active/context.cpp
🧠 Learnings (21)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/governance/signing.cppsrc/rpc/governance.cppsrc/coinjoin/server.cppsrc/node/interfaces.cppsrc/test/util/setup_common.cppsrc/coinjoin/client.cppsrc/coinjoin/coinjoin.cppsrc/wallet/interfaces.cppsrc/chainlock/chainlock.cppsrc/evo/mnauth.cppsrc/instantsend/signing.cppsrc/chainlock/signing.cppsrc/masternode/utils.cppsrc/net.cppsrc/rpc/node.cppsrc/rpc/mining.cppsrc/governance/object.cppsrc/node/sync.hsrc/llmq/quorums.cppsrc/Makefile.amsrc/dsnotificationinterface.cppsrc/init.cppsrc/node/context.cppsrc/governance/vote.cppsrc/instantsend/instantsend.cppsrc/node/sync.cpp
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
Repo: dashpay/dash PR: 6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Applied to files:
src/governance/signing.cppsrc/llmq/quorums.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
src/test/util/setup_common.cpptest/lint/lint-circular-dependencies.py
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Applied to files:
src/test/util/setup_common.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/test/util/setup_common.cppsrc/evo/mnauth.cppsrc/rpc/quorums.cppsrc/llmq/context.cppsrc/node/chainstate.cppsrc/masternode/active/context.cppsrc/net.hsrc/llmq/signing.hsrc/net_signing.cppsrc/instantsend/instantsend.hsrc/net_instantsend.cppsrc/llmq/signing_shares.hsrc/net_processing.cppsrc/llmq/signing.cppsrc/llmq/signing_shares.cpp
📚 Learning: 2025-10-03T11:20:37.475Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/active/context.cpp:29-33
Timestamp: 2025-10-03T11:20:37.475Z
Learning: In Dash codebase, NodeContext (src/node/context.h) serves only as a container with trivial c/d-tors; member lifetime is explicitly managed via reset() calls in the shutdown sequence (src/init.cpp), not by declaration order. For example, active_ctx.reset() is called before DashChainstateSetupClose handles llmq_ctx teardown, ensuring proper destruction order regardless of declaration order in the struct.
Applied to files:
src/test/util/setup_common.cppsrc/node/chainstate.cppsrc/init.cppsrc/node/context.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Applied to files:
src/coinjoin/client.cppsrc/wallet/interfaces.cppsrc/llmq/ehf_signals.cpp
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/wallet/interfaces.cpp
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/instantsend/signing.cppsrc/llmq/context.cppsrc/net_instantsend.hsrc/masternode/active/context.cppsrc/instantsend/instantsend.hsrc/net_instantsend.cppsrc/llmq/signing_shares.hsrc/instantsend/instantsend.cpp
📚 Learning: 2025-07-15T14:53:04.819Z
Learnt from: knst
Repo: dashpay/dash PR: 6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
Applied to files:
src/rpc/quorums.cppsrc/llmq/signing.hsrc/llmq/signing_shares.cpp
📚 Learning: 2025-07-23T09:30:34.631Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Applied to files:
src/node/sync.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/context.cppsrc/llmq/signing_shares.h
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/net_instantsend.hsrc/net_instantsend.cpp
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
src/node/chainstate.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: In src/net_processing.cpp, if ActiveContext (m_active_ctx) is non-null, its members (including cj_server) are guaranteed to be initialized; call sites can safely dereference m_active_ctx->cj_server without an additional null-check.
Applied to files:
src/masternode/active/context.cppsrc/net_processing.cpp
📚 Learning: 2025-10-28T08:54:00.392Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6926
File: test/functional/feature_governance_cl.py:0-0
Timestamp: 2025-10-28T08:54:00.392Z
Learning: In Dash tests, the scheduler (mockscheduler) operates independently of network state. Isolated nodes should still run scheduler-based cleanup processes like governance cleanup, even if they have different state due to network isolation.
Applied to files:
src/init.cppsrc/governance/governance.cpp
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/llmq/signing.hsrc/llmq/signing.cpp
📚 Learning: 2025-08-19T15:08:00.835Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/commitment.cpp:54-60
Timestamp: 2025-08-19T15:08:00.835Z
Learning: In Dash Core, llmq_params.size == 1 is used only for regtest environments, not on public networks, which significantly reduces the risk profile of code paths specific to single-member quorums.
Applied to files:
src/llmq/signing.h
📚 Learning: 2025-09-09T13:04:06.013Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/compat/compat.h:0-0
Timestamp: 2025-09-09T13:04:06.013Z
Learning: WSAEISCONN is only used within WIN32 conditional compilation blocks in the codebase, so it doesn't require errno mapping for non-Windows platforms in src/compat/compat.h.
Applied to files:
src/test/util/net.h
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In PR #6849, `cmapVoteToObject` was intentionally moved from `GovernanceStore` to `CGovernanceManager` as a memory-only variable and is NOT guarded by `cs_store`. It relies on `CacheMap`'s internal thread-safety instead.
Applied to files:
src/governance/governance.cppsrc/governance/governance.h
🧬 Code graph analysis (19)
src/node/sync.h (6)
src/node/sync.cpp (18)
BumpAssetLastTime(41-46)BumpAssetLastTime(41-41)GetAssetName(48-57)GetAssetName(48-48)GetSyncStatus(87-95)GetSyncStatus(87-87)Reset(22-39)Reset(22-22)SwitchToNextAsset(59-85)SwitchToNextAsset(59-59)ProcessMessage(97-110)ProcessMessage(97-97)AcceptedBlockHeader(112-120)AcceptedBlockHeader(112-112)NotifyHeaderTip(122-135)NotifyHeaderTip(122-122)UpdatedBlockTip(137-174)UpdatedBlockTip(137-137)src/governance/governance.cpp (4)
ProcessMessage(183-325)ProcessMessage(183-184)UpdatedBlockTip(1229-1251)UpdatedBlockTip(1229-1229)src/coinjoin/client.cpp (8)
ProcessMessage(41-140)ProcessMessage(41-42)ProcessMessage(142-163)ProcessMessage(142-142)ProcessMessage(178-224)ProcessMessage(178-178)UpdatedBlockTip(1812-1816)UpdatedBlockTip(1812-1812)src/coinjoin/server.cpp (2)
ProcessMessage(26-40)ProcessMessage(26-26)src/evo/mnauth.cpp (2)
ProcessMessage(52-174)ProcessMessage(52-54)src/dsnotificationinterface.cpp (6)
AcceptedBlockHeader(46-50)AcceptedBlockHeader(46-46)NotifyHeaderTip(52-55)NotifyHeaderTip(52-52)UpdatedBlockTip(65-85)UpdatedBlockTip(65-65)
src/net_instantsend.h (2)
src/net_processing.h (1)
PeerManagerInternal(58-72)src/net_instantsend.cpp (12)
ProcessMessage(17-60)ProcessMessage(17-17)Start(62-75)Start(62-62)Stop(77-91)Stop(77-77)WorkThreadMain(241-265)WorkThreadMain(241-241)ProcessPendingISLocks(214-239)ProcessPendingISLocks(214-214)ProcessPendingInstantSendLocks(93-211)ProcessPendingInstantSendLocks(93-95)
src/net_processing.h (6)
src/net_processing.cpp (86)
PeerMisbehaving(6527-6530)PeerMisbehaving(6527-6527)pnode(637-637)pnode(642-642)pnode(652-652)pnode(737-737)pnode(781-781)pnode(853-853)pnode(855-855)PeerIsBanned(6532-6535)PeerIsBanned(6532-6532)node_id(653-653)node_id(748-748)PeerEraseObjectRequest(6537-6540)PeerEraseObjectRequest(6537-6537)nodeid(627-627)nodeid(630-630)nodeid(643-643)nodeid(654-654)nodeid(660-660)nodeid(677-677)nodeid(679-679)nodeid(717-718)nodeid(728-728)nodeid(860-860)nodeid(862-862)nodeid(1038-1038)nodeid(1072-1072)inv(631-631)inv(632-632)inv(655-655)inv(656-656)inv(657-657)inv(669-669)inv(675-675)inv(963-963)inv(2388-2388)PeerRelayInv(6542-6545)PeerRelayInv(6542-6542)PeerRelayInvFiltered(6547-6550)PeerRelayInvFiltered(6547-6547)PeerRelayInvFiltered(6552-6555)PeerRelayInvFiltered(6552-6552)PeerRelayRecoveredSig(6572-6575)PeerRelayRecoveredSig(6572-6572)sig_hash(658-658)PeerAskPeersForTransaction(6557-6560)PeerAskPeersForTransaction(6557-6557)txid(633-633)txid(659-659)txid(663-663)txid(666-666)PeerGetRequestedObjectCount(6562-6565)PeerGetRequestedObjectCount(6562-6562)PeerPostProcessMessage(6567-6570)PeerPostProcessMessage(6567-6567)ret(661-661)ret(683-683)scheduler(623-623)scheduler(649-649)scheduler(692-692)ProcessMessage(3669-5458)ProcessMessage(3669-3674)pfrom(617-617)pfrom(638-639)pfrom(751-753)pfrom(762-762)pfrom(770-770)pfrom(773-773)pfrom(775-775)pfrom(777-777)pfrom(873-873)pfrom(1055-1055)AddExtraHandler(1651-1658)AddExtraHandler(1651-1651)handler(644-644)RemoveHandlers(1660-1665)RemoveHandlers(1660-1660)StartHandlers(1667-1672)StartHandlers(1667-1667)StopHandlers(1674-1682)StopHandlers(1674-1674)InterruptHandlers(1684-1689)InterruptHandlers(1684-1684)ScheduleHandlers(1691-1696)ScheduleHandlers(1691-1691)src/net_instantsend.cpp (7)
inv(186-186)Stop(77-91)Stop(77-77)Start(62-75)Start(62-62)ProcessMessage(17-60)ProcessMessage(17-17)src/init.cpp (3)
Interrupt(230-256)Interrupt(230-230)handler(480-480)src/net_signing.cpp (6)
Stop(156-169)Stop(156-156)Start(144-154)Start(144-144)ProcessMessage(53-142)ProcessMessage(53-53)src/net_governance.cpp (2)
Schedule(21-54)Schedule(21-21)src/coinjoin/walletman.cpp (3)
Schedule(68-75)Schedule(68-68)connman(33-33)
src/llmq/signing.h (5)
src/llmq/signhash.h (3)
llmq(17-52)SignHash(24-46)hash(64-73)src/llmq/signing_shares.cpp (4)
CSigSharesManager(179-189)CSigSharesManager(191-191)Cleanup(1133-1253)Cleanup(1133-1133)src/llmq/signhash.cpp (1)
SignHash(14-22)src/llmq/signing.cpp (17)
CSigningManager(328-332)AlreadyHave(334-347)AlreadyHave(334-334)GetRecoveredSigForGetData(349-359)GetRecoveredSigForGetData(349-349)ProcessRecoveredSig(361-381)ProcessRecoveredSig(361-361)ProcessRecoveredSig(455-494)ProcessRecoveredSig(455-455)FetchPendingReconstructed(447-452)FetchPendingReconstructed(447-447)CollectPendingRecoveredSigsToVerify(383-445)CollectPendingRecoveredSigsToVerify(383-386)GetListeners(496-500)GetListeners(496-496)Cleanup(513-526)Cleanup(513-513)src/net_signing.cpp (2)
ProcessRecoveredSig(171-182)ProcessRecoveredSig(171-171)
src/net_signing.h (4)
src/llmq/signing_shares.h (1)
llmq(35-485)src/llmq/signing.h (1)
CSigningManager(157-230)src/net_processing.h (3)
NetHandler(74-90)NetHandler(78-81)PeerManagerInternal(58-72)src/net_signing.cpp (22)
ProcessMessage(53-142)ProcessMessage(53-53)NotifyRecoveredSig(390-393)NotifyRecoveredSig(390-390)Start(144-154)Start(144-144)Stop(156-169)Stop(156-156)WorkThreadSigning(247-259)WorkThreadSigning(247-247)WorkThreadShares(355-388)WorkThreadShares(355-355)ProcessPendingSigShares(275-353)ProcessPendingSigShares(275-275)ProcessPendingRecoveredSigs(184-245)ProcessPendingRecoveredSigs(184-184)ProcessRecoveredSig(171-182)ProcessRecoveredSig(171-171)BanNode(41-51)BanNode(41-41)RemoveBannedNodeStates(261-271)RemoveBannedNodeStates(261-261)
src/net_governance.h (4)
src/governance/governance.cpp (2)
CGovernanceManager(70-86)CGovernanceManager(88-92)src/node/sync.cpp (1)
CMasternodeSync(14-20)src/net_processing.h (3)
NetHandler(74-90)NetHandler(78-81)PeerManagerInternal(58-72)src/net_governance.cpp (8)
Schedule(21-54)Schedule(21-21)ProcessTick(143-323)ProcessTick(143-143)SendGovernanceSyncRequest(56-61)SendGovernanceSyncRequest(56-56)RequestGovernanceObjectVotes(63-141)RequestGovernanceObjectVotes(63-63)
src/test/util/net.h (1)
src/net_processing.h (1)
PeerManager(105-105)
src/net_governance.cpp (2)
src/net_processing.cpp (9)
inv(631-631)inv(632-632)inv(655-655)inv(656-656)inv(657-657)inv(669-669)inv(675-675)inv(963-963)inv(2388-2388)src/net.h (1)
FullyConnectedOnly(1291-1291)
src/net_signing.cpp (4)
src/llmq/signing_shares.cpp (2)
CSigSharesManager(179-189)CSigSharesManager(191-191)src/llmq/signing.cpp (6)
IsQuorumActive(588-597)IsQuorumActive(588-588)ProcessRecoveredSig(361-381)ProcessRecoveredSig(361-361)ProcessRecoveredSig(455-494)ProcessRecoveredSig(455-455)src/net_processing.cpp (19)
id(701-701)id(705-705)ProcessMessage(3669-5458)ProcessMessage(3669-3674)pfrom(617-617)pfrom(638-639)pfrom(751-753)pfrom(762-762)pfrom(770-770)pfrom(773-773)pfrom(775-775)pfrom(777-777)pfrom(873-873)pfrom(1055-1055)WITH_LOCK(331-334)WITH_LOCK(3234-3234)WITH_LOCK(3256-3256)now(689-689)now(1012-1013)src/net_signing.h (1)
Start(34-36)
src/instantsend/instantsend.h (3)
src/net_instantsend.h (1)
instantsend(12-16)src/instantsend/lock.h (1)
instantsend(18-41)src/instantsend/instantsend.cpp (14)
CInstantSendManager(51-62)CInstantSendManager(64-64)GetCycleBlockHeight(71-78)GetCycleBlockHeight(71-71)IsKnownInstantSend(80-87)IsKnownInstantSend(80-80)EnqueueInstantSendLock(89-110)EnqueueInstantSendLock(89-89)PrepareTxToRetry(391-407)PrepareTxToRetry(391-391)GetPendingLocks(112-143)GetPendingLocks(112-112)ProcessInstantSendLock(145-219)ProcessInstantSendLock(145-146)
src/net_instantsend.cpp (1)
src/net_processing.cpp (25)
ProcessMessage(3669-5458)ProcessMessage(3669-3674)pfrom(617-617)pfrom(638-639)pfrom(751-753)pfrom(762-762)pfrom(770-770)pfrom(773-773)pfrom(775-775)pfrom(777-777)pfrom(873-873)pfrom(1055-1055)id(701-701)id(705-705)ret(661-661)ret(683-683)inv(631-631)inv(632-632)inv(655-655)inv(656-656)inv(657-657)inv(669-669)inv(675-675)inv(963-963)inv(2388-2388)
src/llmq/signing_shares.h (4)
src/llmq/signing.h (2)
CRecoveredSigsListener(149-155)nodiscard(69-91)src/llmq/signing_shares.cpp (34)
CSigSharesManager(179-189)CSigSharesManager(191-191)CreateSigShare(1348-1416)CreateSigShare(1348-1348)ForceReAnnouncement(1419-1443)ForceReAnnouncement(1419-1419)HandleNewRecoveredSig(1445-1450)HandleNewRecoveredSig(1445-1445)AsyncSignIfMember(642-722)AsyncSignIfMember(642-644)FetchPendingSigShares(1319-1324)FetchPendingSigShares(1319-1319)SignPendingSigShare(1326-1346)SignPendingSigShare(1326-1326)SendMessages(973-1000)SendMessages(973-973)Cleanup(1133-1253)Cleanup(1133-1133)MarkAsBanned(1299-1317)MarkAsBanned(1299-1299)RemoveAsBanned(1282-1297)RemoveAsBanned(1282-1282)GetAllNodes(1270-1280)GetAllNodes(1270-1270)ProcessMessageSigShare(334-379)ProcessMessageSigShare(334-334)ProcessSigShare(495-546)ProcessSigShare(495-495)ProcessMessageSigSesAnn(203-233)ProcessMessageSigSesAnn(203-203)ProcessMessageSigShares(235-278)ProcessMessageSigShares(235-235)ProcessMessageBatchedSigShares(280-332)ProcessMessageBatchedSigShares(280-280)src/net_signing.cpp (2)
ProcessMessage(53-142)ProcessMessage(53-53)src/llmq/signhash.h (1)
llmq(17-52)
src/governance/governance.cpp (2)
src/node/interfaces.cpp (8)
hash(853-870)hash(853-853)hash(892-896)hash(892-892)hash(918-923)hash(918-918)vote(149-161)vote(149-149)src/governance/vote.cpp (1)
CGovernanceVote(84-93)
src/governance/governance.h (2)
src/governance/governance.cpp (14)
PrepareVotesToRequest(1056-1096)PrepareVotesToRequest(1056-1056)RequestOrphanObjects(1253-1278)RequestOrphanObjects(1253-1253)CleanOrphanObjects(1280-1296)CleanOrphanObjects(1280-1280)CheckAndRemove(415-534)CheckAndRemove(415-415)GetMNManager(536-539)GetMNManager(536-536)FetchRelayInventory(541-547)FetchRelayInventory(541-541)RequestGovernanceObject(1020-1049)RequestGovernanceObject(1020-1020)src/protocol.h (1)
CInv(531-568)
src/instantsend/instantsend.cpp (3)
src/logging.h (1)
LogAcceptDebug(235-238)src/rpc/quorums.cpp (7)
WITH_LOCK(138-138)WITH_LOCK(993-993)ret(77-77)ret(143-143)ret(185-185)ret(808-808)ret(990-990)src/node/interfaces.cpp (12)
hash(853-870)hash(853-853)hash(892-896)hash(892-892)hash(918-923)hash(918-918)LOCK(541-545)LOCK(551-558)LOCK(559-566)LOCK(828-837)LOCK(871-875)LOCK(1055-1059)
src/node/sync.cpp (3)
src/net.h (1)
AllNodes(1297-1297)src/net_processing.cpp (11)
ProcessMessage(3669-5458)ProcessMessage(3669-3674)peer(697-697)peer(832-832)peer(937-937)peer(938-938)peer(1045-1045)peer(1053-1053)pindexNew(608-608)UpdatedBlockTip(2192-2227)UpdatedBlockTip(2192-2192)src/dsnotificationinterface.cpp (6)
AcceptedBlockHeader(46-50)AcceptedBlockHeader(46-46)NotifyHeaderTip(52-55)NotifyHeaderTip(52-52)UpdatedBlockTip(65-85)UpdatedBlockTip(65-65)
src/net_processing.cpp (3)
src/wallet/interfaces.cpp (2)
scheduler(624-624)scheduler(624-624)src/validationinterface.cpp (4)
RegisterValidationInterface(133-138)RegisterValidationInterface(133-133)UnregisterValidationInterface(145-150)UnregisterValidationInterface(145-145)src/llmq/dkgsessionhandler.cpp (2)
Misbehaving(112-116)Misbehaving(112-112)
src/llmq/signing.cpp (2)
src/llmq/signing.h (1)
CSigningManager(157-230)src/net_signing.cpp (2)
ProcessRecoveredSig(171-182)ProcessRecoveredSig(171-171)
src/llmq/signing_shares.cpp (3)
src/net_processing.cpp (14)
node_id(653-653)node_id(748-748)LOCK(323-329)LOCK(336-340)LOCK(341-345)id(701-701)id(705-705)pnode(637-637)pnode(642-642)pnode(652-652)pnode(737-737)pnode(781-781)pnode(853-853)pnode(855-855)src/spork.h (1)
cs(226-339)src/llmq/signing.cpp (2)
IsQuorumActive(588-597)IsQuorumActive(588-588)
🪛 GitHub Actions: Check Merge Fast-Forward Only
src/governance/governance.cpp
[error] 1-1: Automatic merge failed due to merge conflicts in governance.cpp. Resolve conflicts and commit the result.
src/governance/governance.h
[error] 1-1: Automatic merge failed due to merge conflicts in governance.h. Resolve conflicts and commit the result.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (53)
src/net.h (1)
38-38: Header dependency swap verified as safe.The change replaces
#include <consensus/params.h>with#include <llmq/params.h>. Verification confirms that all symbols used by net.h—specificallyConsensus::LLMQType—are properly defined in llmq/params.h. The refactoring correctly decouples network code from generic consensus parameters while maintaining all necessary functionality. No missing symbols or breakage detected.src/test/util/net.h (1)
27-27: LGTM! Proper use of forward declaration to reduce header dependencies.The forward declaration is appropriate since
PeerManageris only used by reference (line 80) in this header. This eliminates the need to includenet_processing.h, reducing compilation dependencies and supporting the PR's goal of decoupling network/node code from consensus/chain code.src/Makefile.am (1)
298-300: LGTM - New NetHandler headers added.The addition of net_governance.h, net_instantsend.h, and net_signing.h to the public headers aligns with the PR's objective of introducing NetHandler bridge interfaces.
src/llmq/signing_shares.h (5)
360-366: LGTM - Well-designed public struct.The
PendingSignatureDatastruct is cleanly designed with const members ensuring immutability. This promotes the previously internal concept to the public API to support NetSigning integration.
389-389: Verify necessity of RecursiveMutex change.The mutex type changed from
MutextoRecursiveMutex. While this enables re-entrant locking, recursive mutexes can indicate unclear lock hierarchies and are generally discouraged in favor of explicit lock ordering.Please verify:
- Whether the recursive locking is truly necessary for the NetSigning refactor
- If the call graph can be restructured to use a standard
Mutexwith clear lock ordering- That no deadlock scenarios are introduced by the new locking patterns
If recursive locking is required for this refactor, consider documenting the specific call paths that necessitate it.
443-461: LGTM - Public API expansion for NetSigning integration.The expanded public surface with methods like
FetchPendingSigShares(),SignPendingSigShare(),SendMessages(), andCleanup()aligns with the PR's objective of introducing the NetHandler bridge architecture. These methods enable NetSigning to coordinate signature share management without tight coupling.
437-440: All call sites correctly updated.Verification complete. All 5 call sites across the codebase (chainlock/signing.cpp, llmq/ehf_signals.cpp, rpc/quorums.cpp, and instantsend/signing.cpp) correctly use the new
AsyncSignIfMembersignature. Parameters are passed in the correct order, required arguments are provided, and optional parameters are either explicitly passed or use the defaults as intended. No instances of the old signature withCSigningManager¶meter were found.
470-470: No issues found - all call sites handle the new return type correctly.The comprehensive search confirms TryRecoverSig appears only in
src/llmq/signing_shares.h(declaration) andsrc/llmq/signing_shares.cpp(implementation and single call site). The one call site at line 543 correctly forwards the return value to its caller, which properly handles thestd::shared_ptr<CRecoveredSig>return type. The API change has been correctly implemented throughout the codebase.src/chainlock/chainlock.cpp (1)
21-21: LGTM - Header path updated correctly.The include path change from
masternode/sync.htonode/sync.his consistent with the PR's objective of moving synchronization functionality to the node layer.src/rpc/mining.cpp (1)
49-49: LGTM - Header path updated correctly.The include path change from
masternode/sync.htonode/sync.his consistent with the broader refactoring effort to relocate synchronization to the node layer.src/dsnotificationinterface.cpp (1)
21-21: LGTM - Header path updated correctly.The include path change from
masternode/sync.htonode/sync.haligns with the PR's synchronization layer refactoring.src/rpc/governance.cpp (1)
15-15: Include updated to <node/sync.h>Matches the NodeContext usage (node.mn_sync). No further changes needed here.
src/coinjoin/coinjoin.cpp (1)
19-19: Verification successful: no-arg invocation supportedThe
IsTimeOutOfBounds()method in CCoinJoinQueue (src/coinjoin/coinjoin.h:215) accepts an optional parameter with a default value:int64_t current_time = GetAdjustedTime(). This allows the method to be invoked without arguments, confirming the code change is sound and properly layered.src/net.cpp (1)
44-44: Header swap verified and correctThe change is valid.
node/sync.hprovides theCMasternodeSyncclass (defined at line 35) thatnet.cppuses across 8+ functions as parameters. All usages remain compatible, the old header is cleanly removed from the codebase, and the new header is consistently adopted across all 28 dependent files. No compilation issues expected.src/masternode/utils.cpp (1)
13-13: Include path update acknowledged.No issues spotted; ensure build bots cover platforms where header search order differed.
src/chainlock/signing.cpp (2)
13-13: Switched to node/sync.h.Looks consistent with the repo-wide move.
144-144: All AsyncSignIfMember call sites are correctly aligned with the updated signature.The verification confirms:
- Function signature accepts 3 required parameters (llmqType, id, msgHash) plus 3 optional parameters with defaults
- The 3-argument call at
src/chainlock/signing.cpp:144is valid and uses defaults for optional parameters- All five call sites across the codebase correctly use the updated signature
- No stray old-style calls remain
src/masternode/payments.cpp (1)
15-15: Include path update OK.No functional impact observed.
src/coinjoin/client.cpp (1)
13-13: Header move confirmed.CoinJoin client still compiles against CMasternodeSync semantics via node/sync.h? Please confirm with the verification script posted on quorums.cpp.
src/evo/mnauth.cpp (1)
13-13: Include switch LGTM.No other diffs in MNAUTH flow.
src/wallet/interfaces.cpp (1)
35-35: Header move acknowledged.Wallet interfaces unaffected; safe.
src/llmq/quorums.cpp (1)
21-21: Header migration verified complete and clean.Verification confirms no lingering includes of
masternode/sync.hor staleMASTERNODE_SYNC_*constants remain in the codebase. TheCMasternodeSyncclass is properly declared and defined insrc/node/sync.h. The include change at Line 21 is correct.src/governance/signing.cpp (1)
10-10: LGTM! Include path updated for node-layer sync.The include path change from
masternode/sync.htonode/sync.haligns with the PR's objective to relocate synchronization functionality to the node layer.src/node/context.cpp (1)
23-23: LGTM! Include path updated for node-layer sync.The include path change from
masternode/sync.htonode/sync.haligns with the PR's objective to relocate synchronization functionality to the node layer.src/rpc/node.cpp (1)
35-35: LGTM! Include path updated for node-layer sync.The include path change from
masternode/sync.htonode/sync.haligns with the PR's objective to relocate synchronization functionality to the node layer.src/governance/vote.cpp (1)
10-10: LGTM! Include path updated for node-layer sync.The include path change from
masternode/sync.htonode/sync.haligns with the PR's objective to relocate synchronization functionality to the node layer.src/node/interfaces.cpp (1)
28-28: LGTM! Include path updated for node-layer sync.The include path change from
masternode/sync.htonode/sync.haligns with the PR's objective to relocate synchronization functionality to the node layer.src/test/util/setup_common.cpp (1)
20-24: LGTM! New includes added for NetHandler architecture.The new includes for
masternode/active/context.h,net_instantsend.h, andnet_signing.hsupport the NetHandler-based networking components. The update frommasternode/sync.htonode/sync.haligns with the PR's sync relocation objectives.Also applies to: 70-70
src/net_governance.h (1)
1-35: LGTM! Well-designed NetHandler for governance networking.The
NetGovernanceclass follows the NetHandler pattern consistently:
- Properly derives from
NetHandlerand forwardsPeerManagerInternal*to the base- Stores governance dependencies as references (appropriate since ownership is external)
- Marked as
finalto prevent unintended inheritance- Implements the
Schedule()hook for periodic governance sync tasksThe interface is clean and the responsibilities are well-defined.
src/instantsend/instantsend.h (5)
125-138: LGTM! Clean bridge interface for NetInstantSend integration.The new public methods provide a well-structured interface for communication between
CInstantSendManagerandNetInstantSend. The return types are appropriate:
GetCycleBlockHeightreturns negative for unknown cycle_hash (documented)ProcessInstantSendLockusesstd::variantfor flexible return values- Accessors (
Sigman(),Chainstate()) provide controlled access to dependencies
100-100: LGTM! Accessor provides safe access to signer.The
Signer()accessor correctly returns the atomic-loaded pointer, allowing external components to access the signer when needed. Callers should handle the potentialnullptrreturn value appropriately.
8-10: LGTM! Include additions support new functionality.The added includes are necessary for:
<variant>: Return type ofProcessInstantSendLock<instantsend/db.h>,<instantsend/lock.h>,<instantsend/signing.h>: Support new data structures and types used in the interfaceAlso applies to: 22-23
38-41: PendingState data structure change verified successfully.The
PendingState.m_pending_ismember type has been properly updated toUint256HashMap<std::pair<NodeId, instantsend::InstantSendLockPtr>>and all code that accesses this structure is compatible with the new type.Verification confirms:
- Old member
m_pending_peer_activityis completely removed- All operations on
m_pending_isuse standardstd::unordered_mapmethods (size(),emplace(),find(),erase())- The single call site in
net_instantsend.cpp:247correctly unpacks the structure via structured binding and passes the result toProcessPendingISLocks()with matching type and move semantics- Consumer code properly handles the new type throughout
87-89: Constructor call sites are already correctly updated.The single instantiation in
src/llmq/context.cpp:35-36correctly passes all 8 arguments matching the current constructor signature. NoCQuorumManagerparameter exists in either the signature or call site, confirming the refactoring was completed as intended.Likely an incorrect or invalid review comment.
src/llmq/signing.cpp (4)
447-452: LGTM! Efficient extraction of pending reconstructed signatures.The
FetchPendingReconstructedmethod usesswapto efficiently extract pending signatures while holding the lock. This is a clean pattern for moving data out of a protected container.
418-418: LGTM! Structured bindings improve readability.The use of C++17 structured bindings (
auto& [nodeId, v]) is cleaner and more readable than traditional iterator access patterns. This is appropriate for a C++20 codebase.
361-381: LGTM! Call sites correctly handle new return types.Verified that all
ProcessRecoveredSiginvocations properly match their return types:
- Line 67 (NodeId overload): Return value correctly discarded (void)
- Line 173 (const overload): Return value correctly checked as bool
The changes are consistent throughout and no fixes are needed.
328-332: Constructor signature change verified and all call sites updated correctly.The
CSigningManagerconstructor signature has been successfully updated to remove theCChainState¶meter. Verification confirms that the only instantiation site insrc/llmq/context.cpp:33correctly passes three arguments matching the new signature:std::make_unique<llmq::CSigningManager>(*qman, unit_tests, wipe), which corresponds to the three parameters(const CQuorumManager& _qman, bool fMemory, bool fWipe).No issues found.
src/instantsend/instantsend.cpp (6)
71-78: LGTM! Thread-safe implementation with proper error handling.The
GetCycleBlockHeightmethod correctly:
- Uses
WITH_LOCK(::cs_main, ...)for thread-safe access tom_blockman- Returns
-1for unknown cycle hashes (documented in header)- Handles the
nullptrcase safely
80-87: LGTM! Comprehensive check for known InstantSend locks.The method correctly checks both pending locks (with proper synchronization) and the database. The implementation is straightforward and thread-safe.
112-143: LGTM! Efficient batching with proper synchronization.The implementation correctly:
- Limits batch size to 32 items to avoid overwhelming parallel verification (as noted in comment)
- Uses
std::moveto transfer ownership efficiently- Sets
m_pending_workflag when more items remain- Removes processed items after iteration to avoid iterator invalidation
391-407: LGTM! Clean extraction of pending transactions.The method correctly:
- Acquires both locks in a consistent order (
LOCK2)- Returns early if no pending transactions
- Uses structured bindings for clarity
- Only includes transactions with valid
tx_info.tx
66-69: LGTM! Clean extraction of timing reporting logic.The free function
ShouldReportISLockTiming()consolidates the logic for determining when to report timing metrics. This improves code readability and reduces duplication.
145-219: LGTM! Improved return semantics using std::variant.The new return type
std::variant<uint256, CTransactionRef, std::monostate>provides clearer semantics and all call sites properly handle the three cases:
std::monostate: Lock already known or can be ignored (handled via assert)uint256: Transaction ID when TX not found yet—triggers peer lookup for the missing transactionCTransactionRef: Full transaction when lock successfully processed—relayed to peersThe call site in
src/net_instantsend.cpp:187correctly usesstd::holds_alternativeto dispatch on each variant type, ensuring complete coverage of all three return states.src/llmq/signing_shares.cpp (9)
203-233: LGTM! Clean transition to NodeId-based processing.The method signature change from peer reference to
NodeId node_idsuccessfully decouples fromPeerManager. TheNodeIdis used consistently throughout for logging and state access.
548-625: LGTM! Consistent return type with clear success/failure semantics.The method returns
nullptrfor various failure conditions (quorum issues, insufficient shares, invalid signature) andstd::shared_ptr<CRecoveredSig>on success. This is clear and consistent withProcessSigShare.
642-722: LGTM! Clean async signing with proper synchronization.The method correctly:
- Validates quorum membership before signing
- Checks for conflicting votes in the database
- Uses
LOCK(cs_pendingSigns)to safely add topendingSigns- Returns
boolto indicate success/failureThe locking pattern at lines 718-719 is correct for protecting the
pendingSignscontainer.
1270-1280: LGTM! Simple accessor for node IDs.The
GetAllNodesmethod provides a thread-safe way to get all tracked node IDs. The implementation is straightforward and uses proper locking.
1282-1297: LGTM! Clean removal of banned node state.The
RemoveAsBannedmethod correctly:
- Acquires the lock
- Re-requests sig shares from other nodes (via
ForEachcallback)- Removes the node state entirely
This allows recovery after a node is unbanned.
1299-1317: LGTM! Proper handling of banned nodes.The
MarkAsBannedmethod:
- Clears requested shares so they can be requested from other nodes
- Sets the
bannedflag instead of removing state immediately- Allows later cleanup by
Cleanup()methodThis is cleaner than immediate removal and prevents resource leaks.
1319-1346: LGTM! Clean extraction and processing of pending signatures.The new methods support event-driven processing:
FetchPendingSigSharesefficiently extracts pending signatures usingswapSignPendingSigSharecreates, processes, and returns recovered signatures- Proper synchronization with
cs_pendingSignsandcsThese methods enable NetSigning to process signatures on-demand rather than in a dedicated worker thread.
495-546: Return type change properly implemented with correct null handling at all call sites.Verification confirms:
- net_signing.cpp:341: Explicitly checks
if (rs != nullptr)before processing the recovered signature ✓- signing_shares.cpp:1333: Safely returns the result for caller to handle ✓
Both call sites correctly manage the
std::shared_ptr<CRecoveredSig>return type and handle nullptr appropriately. The code change is sound.
179-188: Code is internally consistent; review comment appears based on unavailable AI summary context.The actual constructor signature (lines 179-188) includes the
CSigningManager& _sigmanparameter and properly initializes it viasigman{_sigman}on line 184. The provided code snippet matches the actual file exactly. No inconsistency exists within the code itself. The review comment's concern references an "AI summary" claiming the parameter should have been removed, but without access to that summary or PR context, this cannot be verified. The code as written is self-consistent.
src/Makefile.am
Outdated
| node/miner.h \ | ||
| node/minisketchwrapper.h \ | ||
| node/psbt.h \ | ||
| sync/sync.h \ |
There was a problem hiding this comment.
Incorrect header path - should be node/sync.h.
The header path sync/sync.h is inconsistent with the corresponding source file node/sync.cpp added at Line 577 and the include paths used in other files (#include <node/sync.h>).
Apply this diff:
- sync/sync.h \
+ node/sync.h \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sync/sync.h \ | |
| node/sync.h \ |
🤖 Prompt for AI Agents
In src/Makefile.am around line 318, the listed header path "sync/sync.h" is
incorrect and must match the actual header location used by the source and
includes; update that entry to "node/sync.h" so the Makefile installs/refs the
correct header path and aligns with node/sync.cpp and other files that use
#include <node/sync.h>.
| void NetGovernance::Schedule(CScheduler& scheduler, CConnman& connman) | ||
| { | ||
| if (!m_gov_manager.IsValid()) return; | ||
|
|
||
| scheduler.scheduleEvery( | ||
| [this, &connman]() -> void { | ||
| if (ShutdownRequested()) return; | ||
| ProcessTick(connman); | ||
| }, | ||
| std::chrono::seconds{1}); | ||
|
|
||
| scheduler.scheduleEvery( | ||
| [this, &connman]() -> void { | ||
| if (!m_node_sync.IsSynced()) return; | ||
|
|
||
| // CHECK OBJECTS WE'VE ASKED FOR, REMOVE OLD ENTRIES | ||
| m_gov_manager.CleanOrphanObjects(); | ||
| m_gov_manager.RequestOrphanObjects(connman); | ||
|
|
||
| // CHECK AND REMOVE - REPROCESS GOVERNANCE OBJECTS | ||
| m_gov_manager.CheckAndRemove(); | ||
| }, | ||
| std::chrono::minutes{5}); | ||
|
|
||
| scheduler.scheduleEvery( | ||
| [this]() -> void { | ||
| auto relay_invs = m_gov_manager.FetchRelayInventory(); | ||
| for (const auto& inv : relay_invs) { | ||
| m_peer_manager->PeerRelayInv(inv); | ||
| } | ||
| }, | ||
| // Tests need tighter timings to avoid timeouts, use more relaxed pacing otherwise | ||
| Params().IsMockableChain() ? std::chrono::seconds{1} : std::chrono::seconds{5}); | ||
| } |
There was a problem hiding this comment.
Don’t gate the tick loop on governance availability
Returning immediately when m_gov_manager is invalid prevents us from scheduling ProcessTick() at all. On nodes started with -disablegovernance (or otherwise lacking the DB), this leaves CMasternodeSync stuck in NODE_SYNC_BLOCKCHAIN forever—SwitchToNextAsset() is never reached, mempool sync never fires, and the node never reports itself as synced. Previously the tick ran regardless of governance state, so this is a regression with user-visible fallout. Please keep the 1 s ProcessTick() schedule active unconditionally and only skip the other two maintenance jobs when governance is actually available. A minimal fix would look like:
- if (!m_gov_manager.IsValid()) return;
-
- scheduler.scheduleEvery(
- [this, &connman]() -> void {
- if (ShutdownRequested()) return;
- ProcessTick(connman);
- },
- std::chrono::seconds{1});
+ scheduler.scheduleEvery(
+ [this, &connman]() -> void {
+ if (ShutdownRequested()) return;
+ ProcessTick(connman);
+ },
+ std::chrono::seconds{1});
+
+ if (!m_gov_manager.IsValid()) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void NetGovernance::Schedule(CScheduler& scheduler, CConnman& connman) | |
| { | |
| if (!m_gov_manager.IsValid()) return; | |
| scheduler.scheduleEvery( | |
| [this, &connman]() -> void { | |
| if (ShutdownRequested()) return; | |
| ProcessTick(connman); | |
| }, | |
| std::chrono::seconds{1}); | |
| scheduler.scheduleEvery( | |
| [this, &connman]() -> void { | |
| if (!m_node_sync.IsSynced()) return; | |
| // CHECK OBJECTS WE'VE ASKED FOR, REMOVE OLD ENTRIES | |
| m_gov_manager.CleanOrphanObjects(); | |
| m_gov_manager.RequestOrphanObjects(connman); | |
| // CHECK AND REMOVE - REPROCESS GOVERNANCE OBJECTS | |
| m_gov_manager.CheckAndRemove(); | |
| }, | |
| std::chrono::minutes{5}); | |
| scheduler.scheduleEvery( | |
| [this]() -> void { | |
| auto relay_invs = m_gov_manager.FetchRelayInventory(); | |
| for (const auto& inv : relay_invs) { | |
| m_peer_manager->PeerRelayInv(inv); | |
| } | |
| }, | |
| // Tests need tighter timings to avoid timeouts, use more relaxed pacing otherwise | |
| Params().IsMockableChain() ? std::chrono::seconds{1} : std::chrono::seconds{5}); | |
| } | |
| void NetGovernance::Schedule(CScheduler& scheduler, CConnman& connman) | |
| { | |
| scheduler.scheduleEvery( | |
| [this, &connman]() -> void { | |
| if (ShutdownRequested()) return; | |
| ProcessTick(connman); | |
| }, | |
| std::chrono::seconds{1}); | |
| if (!m_gov_manager.IsValid()) return; | |
| scheduler.scheduleEvery( | |
| [this, &connman]() -> void { | |
| if (!m_node_sync.IsSynced()) return; | |
| // CHECK OBJECTS WE'VE ASKED FOR, REMOVE OLD ENTRIES | |
| m_gov_manager.CleanOrphanObjects(); | |
| m_gov_manager.RequestOrphanObjects(connman); | |
| // CHECK AND REMOVE - REPROCESS GOVERNANCE OBJECTS | |
| m_gov_manager.CheckAndRemove(); | |
| }, | |
| std::chrono::minutes{5}); | |
| scheduler.scheduleEvery( | |
| [this]() -> void { | |
| auto relay_invs = m_gov_manager.FetchRelayInventory(); | |
| for (const auto& inv : relay_invs) { | |
| m_peer_manager->PeerRelayInv(inv); | |
| } | |
| }, | |
| // Tests need tighter timings to avoid timeouts, use more relaxed pacing otherwise | |
| Params().IsMockableChain() ? std::chrono::seconds{1} : std::chrono::seconds{5}); | |
| } |
🤖 Prompt for AI Agents
In src/net_governance.cpp around lines 21 to 54, the current early return when
m_gov_manager.IsValid() is false prevents scheduling the 1s ProcessTick() job
and stalls node sync; change the logic so the 1-second scheduler for
ProcessTick(connman) is always registered unconditionally, and only gate the two
governance-related scheduled tasks (the 5-minute Clean/Request/Check task and
the relay inventory task) on m_gov_manager.IsValid(); remove the top-level
return and wrap the other two scheduler.scheduleEvery(...) calls in an if
(m_gov_manager.IsValid()) { ... } (or equivalent) so governance-dependent jobs
are skipped when invalid but ProcessTick still runs.
| nTimeLastProcess = GetTime(); | ||
| const CConnman::NodesSnapshot snap{connman, /* cond = */ CConnman::FullyConnectedOnly}; | ||
|
|
||
| // gradually request the rest of the votes after sync finished | ||
| if(m_node_sync.IsSynced()) { | ||
| RequestGovernanceObjectVotes(snap.Nodes(), connman); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Skip vote requests when governance is disabled
Once the tick runs even without governance, this IsSynced() early return will still invoke RequestGovernanceObjectVotes, which dereferences the governance manager. On a node started with -disablegovernance, that’s undefined behaviour. Guarding this block fixes the issue:
- if(m_node_sync.IsSynced()) {
+ if(!m_gov_manager.IsValid()) {
+ return;
+ }
+ if (m_node_sync.IsSynced()) {
RequestGovernanceObjectVotes(snap.Nodes(), connman);
return;
}This keeps the sync state machine advancing while cleanly bailing out of vote polling when governance is unavailable.
🤖 Prompt for AI Agents
In src/net_governance.cpp around lines 167 to 174, when the node is synced the
code unconditionally calls RequestGovernanceObjectVotes which dereferences the
governance manager and causes undefined behaviour when governance is disabled;
update the early-return block to first check whether governance is
enabled/available (e.g., guard with the governance manager presence or a
disable-governance flag) and only call RequestGovernanceObjectVotes if
governance is enabled, otherwise skip the vote-request call while still allowing
the sync state machine to advance.
| auto llmqType = Params().GetConsensus().llmqTypeDIP0024InstantSend; | ||
| const auto& llmq_params_opt = Params().GetLLMQ(llmqType); | ||
| assert(llmq_params_opt); | ||
| const auto& llmq_params = llmq_params_opt.value(); | ||
| auto dkgInterval = llmq_params.dkgInterval; |
There was a problem hiding this comment.
Prevent crash when the DIP24 quorum is unavailable.
When llmqTypeDIP0024InstantSend is LLMQ_NONE (e.g., in regtest/unit-test setups or any network that has not configured a DIP24 quorum yet), Params().GetLLMQ(llmqType) returns std::nullopt. Calling .value() in either ProcessPendingISLocks or ProcessMessage will then throw std::bad_optional_access, terminating the node. Please guard the optional before dereferencing and bail out early if no quorum is configured.
auto llmqType = Params().GetConsensus().llmqTypeDIP0024InstantSend;
- const auto& llmq_params_opt = Params().GetLLMQ(llmqType);
- assert(llmq_params_opt);
- const auto& llmq_params = llmq_params_opt.value();
+ const auto llmq_params_opt = Params().GetLLMQ(llmqType);
+ if (!llmq_params_opt.has_value()) {
+ return;
+ }
+ const auto& llmq_params = *llmq_params_opt;(The same guard should be applied in ProcessMessage.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto llmqType = Params().GetConsensus().llmqTypeDIP0024InstantSend; | |
| const auto& llmq_params_opt = Params().GetLLMQ(llmqType); | |
| assert(llmq_params_opt); | |
| const auto& llmq_params = llmq_params_opt.value(); | |
| auto dkgInterval = llmq_params.dkgInterval; | |
| auto llmqType = Params().GetConsensus().llmqTypeDIP0024InstantSend; | |
| const auto llmq_params_opt = Params().GetLLMQ(llmqType); | |
| if (!llmq_params_opt.has_value()) { | |
| return; | |
| } | |
| const auto& llmq_params = *llmq_params_opt; | |
| auto dkgInterval = llmq_params.dkgInterval; |
🤖 Prompt for AI Agents
In src/net_instantsend.cpp around lines 217 to 221, the code calls
llmq_params_opt.value() without checking the optional, which crashes when the
DIP24 quorum is not configured (std::bad_optional_access); update the code to
check the optional before dereferencing (e.g., if (!llmq_params_opt) return or
otherwise bail out/skip InstantSend processing when llmqTypeDIP0024InstantSend
is LLMQ_NONE or GetLLMQ returns nullopt), and apply the identical guard in
ProcessMessage to avoid accessing .value() when the quorum is unavailable.
| void PeerManagerImpl::PeerPostProcessMessage(MessageProcessingResult&& ret) | ||
| { | ||
| PostProcessMessage(std::move(ret), -1); | ||
| } |
There was a problem hiding this comment.
Incorrect peer id passed through PeerPostProcessMessage
PeerPostProcessMessage now forwards every result to PostProcessMessage with node = -1. That short-circuits key paths: Misbehaving(-1, …) bails out, so handlers can no longer raise bans, and EraseObjectRequest(-1, …) never clears g_already_asked_for, leaving stale getdata bookkeeping and blocking future retries. This regresses existing DoS protection and request accounting, so we must propagate the real peer id.
Please change the interface/implementation to pass the originating NodeId down (similar to the direct call sites above), for example:
-void PeerPostProcessMessage(MessageProcessingResult&& ret) override {
- PostProcessMessage(std::move(ret), -1);
-}
+void PeerPostProcessMessage(NodeId nodeid, MessageProcessingResult&& ret) override {
+ PostProcessMessage(std::move(ret), nodeid);
+}and adjust callers accordingly so handlers supply the true peer id.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 6567-6570, PeerPostProcessMessage
currently forwards results to PostProcessMessage with node = -1 which prevents
Misbehaving() and EraseObjectRequest() from operating on the real peer; update
the PeerPostProcessMessage signature and body to accept and pass the originating
NodeId through to PostProcessMessage (i.e. replace the hardcoded -1 with the
provided node id), then update all callers of PeerPostProcessMessage to supply
the true NodeId from their context so handlers can correctly record misbehavior
and clear g_already_asked_for.
| #include <bls/bls_batchverifier.h> | ||
| #include <cxxtimer.hpp> | ||
| #include <llmq/commitment.h> | ||
| #include <llmq/quorums.h> | ||
| #include <llmq/signhash.h> | ||
| #include <llmq/signing.h> | ||
| #include <llmq/signing_shares.h> | ||
| #include <masternode/node.h> | ||
| #include <logging.h> | ||
| #include <spork.h> | ||
| #include <streams.h> | ||
| #include <util/thread.h> | ||
| #include <validationinterface.h> | ||
|
|
||
| #include <unordered_map> | ||
|
|
||
| using llmq::CSigSharesManager; | ||
|
|
||
| static bool PreVerifyRecoveredSig(Consensus::LLMQType& llmqType, const llmq::CQuorumManager& quorum_manager, const llmq::CRecoveredSig& recoveredSig) | ||
| { |
There was a problem hiding this comment.
Include util/ranges.h before using ranges::all_of
This new file relies on the ranges::all_of alias, which Dash exposes via <util/ranges.h>. Without that header the compiler reports “namespace ‘ranges’ has not been declared”. Please pull in the helper before using the alias.
#include <llmq/signing.h>
#include <llmq/signing_shares.h>
#include <masternode/node.h>
#include <logging.h>
+#include <util/ranges.h>
#include <spork.h>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <bls/bls_batchverifier.h> | |
| #include <cxxtimer.hpp> | |
| #include <llmq/commitment.h> | |
| #include <llmq/quorums.h> | |
| #include <llmq/signhash.h> | |
| #include <llmq/signing.h> | |
| #include <llmq/signing_shares.h> | |
| #include <masternode/node.h> | |
| #include <logging.h> | |
| #include <spork.h> | |
| #include <streams.h> | |
| #include <util/thread.h> | |
| #include <validationinterface.h> | |
| #include <unordered_map> | |
| using llmq::CSigSharesManager; | |
| static bool PreVerifyRecoveredSig(Consensus::LLMQType& llmqType, const llmq::CQuorumManager& quorum_manager, const llmq::CRecoveredSig& recoveredSig) | |
| { | |
| #include <bls/bls_batchverifier.h> | |
| #include <cxxtimer.hpp> | |
| #include <llmq/commitment.h> | |
| #include <llmq/quorums.h> | |
| #include <llmq/signhash.h> | |
| #include <llmq/signing.h> | |
| #include <llmq/signing_shares.h> | |
| #include <masternode/node.h> | |
| #include <logging.h> | |
| #include <util/ranges.h> | |
| #include <spork.h> | |
| #include <streams.h> | |
| #include <util/thread.h> | |
| #include <validationinterface.h> | |
| #include <unordered_map> | |
| using llmq::CSigSharesManager; | |
| static bool PreVerifyRecoveredSig(Consensus::LLMQType& llmqType, const llmq::CQuorumManager& quorum_manager, const llmq::CRecoveredSig& recoveredSig) | |
| { |
🤖 Prompt for AI Agents
In src/net_signing.cpp around lines 7 to 26, the file uses the ranges::all_of
alias but does not include Dash's helper header; add #include "util/ranges.h" to
the include block (place it with the other util headers, e.g. after
<util/thread.h>) so ranges::all_of is declared and the file compiles cleanly.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR restructures node synchronization and network messaging infrastructure across five primary areas. First, it relocates masternode synchronization to the node layer (moving masternode/sync.h → node/sync.h) and renames associated constants from MASTERNODE_SYNC_* to NODE_SYNC_*. Second, it introduces a new NetHandler-based plugin architecture where peer message processing is delegated to pluggable handlers (NetGovernance, NetInstantSend, NetSigning) managed by a new PeerManagerInternal interface. Third, it refactors CGovernanceManager by removing the Schedule method, replacing RequestGovernanceObjectVotes with PrepareVotesToRequest, and updating governance object lookup patterns. Fourth, it substantially redesigns CInstantSendManager and CSigSharesManager by removing CQuorumManager and PeerManager dependencies respectively, shifting to NodeId-centric APIs and eliminating worker threads in favor of explicit synchronous processing. Fifth, it updates the build system and removes the legacy CMasternodeSync implementation, consolidating synchronization logic into the relocated node/sync.h header. Sequence DiagramsequenceDiagram
actor App as Application<br/>Startup
participant PM as PeerManager
participant NH as NetHandler<br/>(Gov/IS/Signing)
participant GM as CGovernanceManager
participant ISM as CInstantSendManager
participant SM as CSigningManager
App->>PM: AddExtraHandler(NetGovernance)
App->>PM: AddExtraHandler(NetInstantSend)
App->>PM: AddExtraHandler(NetSigning)
rect rgb(200, 220, 255)
note over App,SM: Startup & Handler Lifecycle
App->>PM: StartHandlers()
PM->>NH: Start()
Note over NH: Initialize worker threads<br/>(if applicable)
App->>PM: ScheduleHandlers(scheduler)
PM->>NH: Schedule(scheduler, connman)
Note over NH: Register periodic tasks<br/>(e.g., governance tick)
end
rect rgb(200, 255, 220)
note over App,SM: Message Processing Flow
App->>PM: ProcessMessage(node, type, data)
PM->>NH: ProcessMessage(node, type, data)
alt Message Type Handled
NH->>GM: PrepareVotesToRequest(...) or<br/>ProcessTick(...)
NH->>ISM: ProcessInstantSendLock(...) returns variant
NH->>SM: ProcessRecoveredSig(...) returns bool
else Message Type Not Handled
NH-->>PM: Return unhandled
end
end
rect rgb(255, 240, 200)
note over App,SM: Governance Sync Flow
Note over NH: NetGovernance::ProcessTick()
NH->>GM: PrepareVotesToRequest(nodes, asked_map, now, peers_max)
activate GM
Note over GM: Compute vote hashes to request<br/>respecting throttle state
GM-->>NH: Return (hashes_to_request, hashes_remaining)
deactivate GM
NH->>PM: PeerRelayInv(vote_inv) for each peer
end
rect rgb(255, 220, 220)
note over App,SM: Shutdown
App->>PM: StopHandlers()
PM->>NH: Stop()
Note over NH: Stop worker threads,<br/>cleanup state
App->>PM: RemoveHandlers()
Note over PM: Clear handler list
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
src/net_processing.cpp (1)
598-601: About the commented-out destructor codeThe comment-only destructor was previously flagged. Beyond style, it must actively call RemoveHandlers() to safely unregister validation callbacks. Please implement the destructor as in the earlier diff rather than leaving commented code.
🧹 Nitpick comments (8)
src/net_processing.cpp (5)
1092-1092: Guard access to m_handlers to avoid shutdown racesm_handlers is mutated in RemoveHandlers() and iterated in ProcessMessage() (Line 5448). Add a small mutex (or SharedMutex) around reads/writes to prevent iterator invalidation during shutdown.
Apply this minimal pattern:
- std::vector<std::unique_ptr<NetHandler>> m_handlers; + mutable std::mutex m_handlers_mutex; + std::vector<std::unique_ptr<NetHandler>> m_handlers;And take the lock in Add/Remove/Start/Stop/Interrupt/Schedule and in the ProcessMessage loop that iterates handlers. Example for iteration (see Line 5448):
- for (const auto& handler : m_handlers) { + std::lock_guard<std::mutex> lk(m_handlers_mutex); + for (const auto& handler : m_handlers) { handler->ProcessMessage(pfrom, msg_type, vRecv); }
1674-1682: StopHandlers() order is good; add brief commentYou unregister before Stop(), which prevents callbacks during teardown. Consider a short comment documenting this ordering to avoid future regressions.
1684-1697: Schedule/Interrupt/Start: consider lockingWrap loops over m_handlers with the proposed m_handlers_mutex to avoid concurrent modification during shutdown.
5448-5451: Handler dispatch placement and stream reuse
- Dispatch is gated by “found” net message types above; this blocks handler processing of extension messages not listed in getAllNetMessageTypes(). Call handlers before the unknown-command fallback so plugins can handle new types.
- vRecv is passed by reference to many subsystems and then to all handlers. If two handlers process the same msg_type, the first may consume vRecv, starving the next. If multiple handlers are expected to observe the same payload, pass a copy per handler or document exclusivity.
Option A (call handlers unconditionally, before unknown fallback):
- if (found) - { + if (found) { ... - for (const auto& handler : m_handlers) { - handler->ProcessMessage(pfrom, msg_type, vRecv); - } - return; - } + { + std::lock_guard<std::mutex> lk(m_handlers_mutex); + for (const auto& handler : m_handlers) { + handler->ProcessMessage(pfrom, msg_type, vRecv); + } + } + return; + } + // Give handlers a chance to process unknown/extension messages + { + std::lock_guard<std::mutex> lk(m_handlers_mutex); + for (const auto& handler : m_handlers) { + handler->ProcessMessage(pfrom, msg_type, vRecv); + } + } + // Fallback if unhandled:If multiple handlers must process the same payload, consider:
- handler->ProcessMessage(pfrom, msg_type, vRecv); + CDataStream tmp = vRecv; // shallow copy of the stream buffer + handler->ProcessMessage(pfrom, msg_type, tmp);Please confirm the intended handler model (single owner vs fanout). If fanout is intended, adopt the copy approach; if single-owner, document it to avoid future double-consumption bugs.
6526-6575: Preserve peer attribution in PeerPostProcessMessagePeerPostProcessMessage() forwards with node = -1, so errors and erasures aren’t attributed to the source peer. Prefer passing the NodeId (or extend the interface to accept it) so Misbehaving/EraseObjectRequest can act on the correct peer.
Possible interface tweak:
- void PeerPostProcessMessage(MessageProcessingResult&& ret) override; + void PeerPostProcessMessage(NodeId nodeid, MessageProcessingResult&& ret) override;And
- PostProcessMessage(std::move(ret), -1); + PostProcessMessage(std::move(ret), nodeid);src/llmq/signing_shares.h (3)
442-464: Consider further refinement in final implementation.The substantial expansion of the public API surface successfully decouples from PeerManager but creates extensive method exposure. The class shifts from an encapsulated manager with internal workflow to a collection of utilities orchestrated externally by NetSigning.
For the final implementation, consider:
- Whether some of these methods could be grouped into smaller, focused interfaces
- If the exposed internal state management creates fragile coupling through shared lock requirements
- Whether helper classes could encapsulate related operations (e.g., node state management, sig share collection)
Given this is a proof-of-concept refactor, the current approach is reasonable for validating the architecture direction.
447-448: Consider direct dependency injection in final version.These accessor methods expose internal dependencies through
CSigSharesManager. For the final implementation, consider whetherNetSigningshould receiveCActiveMasternodeManagerandCSporkManagerdirectly via dependency injection rather than accessing them throughCSigSharesManager. This would reduce indirection and make dependencies more explicit.For this proof-of-concept refactor, the current approach is acceptable.
389-484: Verify thread-safety coordination in final implementation.The lock annotations are correctly specified, with
EXCLUSIVE_LOCKS_REQUIRED(!cs)indicating methods that internally acquire locks. However, the expanded public API creates additional thread-safety coordination requirements betweenCSigSharesManagerandNetSigning.For the final implementation, ensure:
- NetSigning doesn't introduce new locks that could create deadlock potential when calling into CSigSharesManager
- The locking discipline is documented clearly for maintainers
- Consider whether the recursive mutex is necessary or if simpler locking patterns could be achieved with the new architecture
The current proof-of-concept approach appears reasonable based on the provided context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
src/Makefile.am(4 hunks)src/chainlock/chainlock.cpp(1 hunks)src/chainlock/signing.cpp(2 hunks)src/coinjoin/client.cpp(1 hunks)src/coinjoin/coinjoin.cpp(1 hunks)src/coinjoin/server.cpp(1 hunks)src/dsnotificationinterface.cpp(1 hunks)src/evo/mnauth.cpp(1 hunks)src/governance/governance.cpp(15 hunks)src/governance/governance.h(3 hunks)src/governance/object.cpp(1 hunks)src/governance/signing.cpp(1 hunks)src/governance/vote.cpp(1 hunks)src/init.cpp(7 hunks)src/instantsend/instantsend.cpp(8 hunks)src/instantsend/instantsend.h(5 hunks)src/instantsend/signing.cpp(3 hunks)src/llmq/context.cpp(1 hunks)src/llmq/ehf_signals.cpp(2 hunks)src/llmq/quorums.cpp(1 hunks)src/llmq/signing.cpp(8 hunks)src/llmq/signing.h(5 hunks)src/llmq/signing_shares.cpp(25 hunks)src/llmq/signing_shares.h(3 hunks)src/masternode/active/context.cpp(1 hunks)src/masternode/active/notificationinterface.cpp(0 hunks)src/masternode/active/notificationinterface.h(0 hunks)src/masternode/payments.cpp(1 hunks)src/masternode/sync.cpp(0 hunks)src/masternode/utils.cpp(1 hunks)src/msg_result.h(0 hunks)src/net.cpp(1 hunks)src/net.h(1 hunks)src/net_governance.cpp(1 hunks)src/net_governance.h(1 hunks)src/net_instantsend.cpp(1 hunks)src/net_instantsend.h(1 hunks)src/net_processing.cpp(7 hunks)src/net_processing.h(2 hunks)src/net_signing.cpp(1 hunks)src/net_signing.h(1 hunks)src/node/chainstate.cpp(2 hunks)src/node/context.cpp(1 hunks)src/node/interfaces.cpp(1 hunks)src/node/sync.cpp(1 hunks)src/node/sync.h(4 hunks)src/rpc/governance.cpp(1 hunks)src/rpc/mining.cpp(1 hunks)src/rpc/node.cpp(1 hunks)src/rpc/quorums.cpp(1 hunks)src/test/util/net.h(1 hunks)src/test/util/setup_common.cpp(3 hunks)src/validation.h(0 hunks)src/wallet/interfaces.cpp(1 hunks)test/lint/lint-circular-dependencies.py(2 hunks)
💤 Files with no reviewable changes (5)
- src/masternode/active/notificationinterface.h
- src/validation.h
- src/masternode/active/notificationinterface.cpp
- src/msg_result.h
- src/masternode/sync.cpp
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/rpc/node.cppsrc/net.hsrc/wallet/interfaces.cppsrc/rpc/governance.cppsrc/masternode/payments.cppsrc/coinjoin/server.cppsrc/coinjoin/coinjoin.cppsrc/governance/object.cppsrc/node/interfaces.cppsrc/dsnotificationinterface.cppsrc/masternode/active/context.cppsrc/test/util/setup_common.cppsrc/net_instantsend.hsrc/rpc/mining.cppsrc/test/util/net.hsrc/node/chainstate.cppsrc/llmq/quorums.cppsrc/net_signing.cppsrc/evo/mnauth.cppsrc/rpc/quorums.cppsrc/coinjoin/client.cppsrc/chainlock/chainlock.cppsrc/net_governance.cppsrc/net_signing.hsrc/net.cppsrc/llmq/signing.cppsrc/chainlock/signing.cppsrc/llmq/context.cppsrc/masternode/utils.cppsrc/node/context.cppsrc/llmq/ehf_signals.cppsrc/governance/vote.cppsrc/node/sync.hsrc/instantsend/instantsend.cppsrc/net_processing.hsrc/node/sync.cppsrc/net_processing.cppsrc/governance/governance.cppsrc/net_instantsend.cppsrc/net_governance.hsrc/llmq/signing_shares.cppsrc/llmq/signing.hsrc/governance/signing.cppsrc/instantsend/signing.cppsrc/instantsend/instantsend.hsrc/governance/governance.hsrc/init.cppsrc/llmq/signing_shares.h
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/payments.cppsrc/masternode/active/context.cppsrc/evo/mnauth.cppsrc/masternode/utils.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/util/setup_common.cppsrc/test/util/net.h
🧠 Learnings (19)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/rpc/node.cppsrc/wallet/interfaces.cppsrc/rpc/governance.cppsrc/masternode/payments.cppsrc/coinjoin/server.cppsrc/coinjoin/coinjoin.cppsrc/governance/object.cppsrc/node/interfaces.cppsrc/test/util/setup_common.cppsrc/rpc/mining.cppsrc/llmq/quorums.cppsrc/evo/mnauth.cppsrc/coinjoin/client.cppsrc/chainlock/chainlock.cppsrc/net.cppsrc/chainlock/signing.cppsrc/masternode/utils.cppsrc/governance/vote.cppsrc/node/sync.hsrc/instantsend/instantsend.cppsrc/Makefile.amsrc/governance/signing.cppsrc/instantsend/signing.cppsrc/init.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/net.hsrc/masternode/active/context.cppsrc/test/util/setup_common.cppsrc/node/chainstate.cppsrc/net_signing.cppsrc/evo/mnauth.cppsrc/llmq/signing.cppsrc/llmq/context.cppsrc/net_processing.cppsrc/net_instantsend.cppsrc/llmq/signing_shares.cppsrc/llmq/signing.hsrc/instantsend/instantsend.hsrc/llmq/signing_shares.h
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Applied to files:
src/wallet/interfaces.cppsrc/coinjoin/client.cppsrc/llmq/ehf_signals.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: In src/net_processing.cpp, if ActiveContext (m_active_ctx) is non-null, its members (including cj_server) are guaranteed to be initialized; call sites can safely dereference m_active_ctx->cj_server without an additional null-check.
Applied to files:
src/masternode/active/context.cppsrc/net_processing.cpp
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/masternode/active/context.cppsrc/net_instantsend.hsrc/llmq/context.cppsrc/instantsend/instantsend.cppsrc/net_instantsend.cppsrc/instantsend/signing.cppsrc/instantsend/instantsend.hsrc/llmq/signing_shares.h
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
src/test/util/setup_common.cppsrc/node/chainstate.cpp
📚 Learning: 2025-10-03T11:20:37.475Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/active/context.cpp:29-33
Timestamp: 2025-10-03T11:20:37.475Z
Learning: In Dash codebase, NodeContext (src/node/context.h) serves only as a container with trivial c/d-tors; member lifetime is explicitly managed via reset() calls in the shutdown sequence (src/init.cpp), not by declaration order. For example, active_ctx.reset() is called before DashChainstateSetupClose handles llmq_ctx teardown, ensuring proper destruction order regardless of declaration order in the struct.
Applied to files:
src/test/util/setup_common.cppsrc/node/context.cppsrc/init.cpp
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/net_instantsend.hsrc/net_instantsend.cpp
📚 Learning: 2025-09-09T13:04:06.013Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/compat/compat.h:0-0
Timestamp: 2025-09-09T13:04:06.013Z
Learning: WSAEISCONN is only used within WIN32 conditional compilation blocks in the codebase, so it doesn't require errno mapping for non-Windows platforms in src/compat/compat.h.
Applied to files:
src/test/util/net.h
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
src/node/chainstate.cpp
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
Repo: dashpay/dash PR: 6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Applied to files:
src/llmq/quorums.cppsrc/governance/signing.cpp
📚 Learning: 2025-07-15T14:53:04.819Z
Learnt from: knst
Repo: dashpay/dash PR: 6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
Applied to files:
src/rpc/quorums.cppsrc/llmq/signing_shares.cppsrc/llmq/signing.h
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/llmq/signing.cppsrc/chainlock/signing.cppsrc/llmq/signing.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/context.cppsrc/llmq/signing_shares.h
📚 Learning: 2025-07-23T09:30:34.631Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Applied to files:
src/node/sync.h
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In PR #6849, `cmapVoteToObject` was intentionally moved from `GovernanceStore` to `CGovernanceManager` as a memory-only variable and is NOT guarded by `cs_store`. It relies on `CacheMap`'s internal thread-safety instead.
Applied to files:
src/governance/governance.cpp
📚 Learning: 2025-10-28T08:54:00.392Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6926
File: test/functional/feature_governance_cl.py:0-0
Timestamp: 2025-10-28T08:54:00.392Z
Learning: In Dash tests, the scheduler (mockscheduler) operates independently of network state. Isolated nodes should still run scheduler-based cleanup processes like governance cleanup, even if they have different state due to network isolation.
Applied to files:
src/governance/governance.cppsrc/init.cpp
📚 Learning: 2025-08-19T15:08:00.835Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/commitment.cpp:54-60
Timestamp: 2025-08-19T15:08:00.835Z
Learning: In Dash Core, llmq_params.size == 1 is used only for regtest environments, not on public networks, which significantly reduces the risk profile of code paths specific to single-member quorums.
Applied to files:
src/llmq/signing.h
🧬 Code graph analysis (18)
src/net_instantsend.h (3)
src/instantsend/instantsend.h (1)
instantsend(35-41)src/net_processing.h (1)
PeerManagerInternal(58-72)src/net_instantsend.cpp (12)
ProcessMessage(17-60)ProcessMessage(17-17)Start(62-75)Start(62-62)Stop(77-91)Stop(77-77)WorkThreadMain(241-265)WorkThreadMain(241-241)ProcessPendingISLocks(214-239)ProcessPendingISLocks(214-214)ProcessPendingInstantSendLocks(93-211)ProcessPendingInstantSendLocks(93-95)
src/test/util/net.h (1)
src/net_processing.h (1)
PeerManager(105-105)
src/net_signing.cpp (5)
src/llmq/signing_shares.cpp (2)
CSigSharesManager(179-189)CSigSharesManager(191-191)src/llmq/signing.cpp (6)
IsQuorumActive(588-597)IsQuorumActive(588-588)ProcessRecoveredSig(361-381)ProcessRecoveredSig(361-361)ProcessRecoveredSig(455-494)ProcessRecoveredSig(455-455)src/net_processing.cpp (30)
id(701-701)id(705-705)ProcessMessage(3669-5458)ProcessMessage(3669-3674)pfrom(617-617)pfrom(638-639)pfrom(751-753)pfrom(762-762)pfrom(770-770)pfrom(773-773)pfrom(775-775)pfrom(777-777)pfrom(873-873)pfrom(1055-1055)WITH_LOCK(331-334)WITH_LOCK(3234-3234)WITH_LOCK(3256-3256)inv(631-631)inv(632-632)inv(655-655)inv(656-656)inv(657-657)inv(669-669)inv(675-675)inv(963-963)inv(2388-2388)node_id(653-653)node_id(748-748)now(689-689)now(1012-1013)src/masternode/active/context.cpp (4)
Start(53-57)Start(53-53)Stop(59-63)Stop(59-59)src/net_signing.h (1)
Start(34-36)
src/net_governance.cpp (2)
src/net_processing.cpp (16)
inv(631-631)inv(632-632)inv(655-655)inv(656-656)inv(657-657)inv(669-669)inv(675-675)inv(963-963)inv(2388-2388)pnode(637-637)pnode(642-642)pnode(652-652)pnode(737-737)pnode(781-781)pnode(853-853)pnode(855-855)src/net.h (1)
FullyConnectedOnly(1291-1291)
src/net_signing.h (4)
src/llmq/signing_shares.h (1)
llmq(35-485)src/llmq/signing.h (1)
CSigningManager(157-230)src/net_processing.h (3)
NetHandler(74-90)NetHandler(78-81)PeerManagerInternal(58-72)src/net_signing.cpp (22)
ProcessMessage(53-142)ProcessMessage(53-53)NotifyRecoveredSig(390-393)NotifyRecoveredSig(390-390)Start(144-154)Start(144-144)Stop(156-169)Stop(156-156)WorkThreadSigning(247-259)WorkThreadSigning(247-247)WorkThreadShares(355-388)WorkThreadShares(355-355)ProcessPendingSigShares(275-353)ProcessPendingSigShares(275-275)ProcessPendingRecoveredSigs(184-245)ProcessPendingRecoveredSigs(184-184)ProcessRecoveredSig(171-182)ProcessRecoveredSig(171-171)BanNode(41-51)BanNode(41-41)RemoveBannedNodeStates(261-271)RemoveBannedNodeStates(261-261)
src/llmq/signing.cpp (2)
src/llmq/signing.h (1)
CSigningManager(157-230)src/net_signing.cpp (2)
ProcessRecoveredSig(171-182)ProcessRecoveredSig(171-171)
src/node/sync.h (2)
src/node/sync.cpp (18)
BumpAssetLastTime(41-46)BumpAssetLastTime(41-41)GetAssetName(48-57)GetAssetName(48-48)GetSyncStatus(87-95)GetSyncStatus(87-87)Reset(22-39)Reset(22-22)SwitchToNextAsset(59-85)SwitchToNextAsset(59-59)ProcessMessage(97-110)ProcessMessage(97-97)AcceptedBlockHeader(112-120)AcceptedBlockHeader(112-112)NotifyHeaderTip(122-135)NotifyHeaderTip(122-122)UpdatedBlockTip(137-174)UpdatedBlockTip(137-137)src/dsnotificationinterface.cpp (6)
AcceptedBlockHeader(46-50)AcceptedBlockHeader(46-46)NotifyHeaderTip(52-55)NotifyHeaderTip(52-52)UpdatedBlockTip(65-85)UpdatedBlockTip(65-65)
src/instantsend/instantsend.cpp (2)
src/logging.h (1)
LogAcceptDebug(235-238)src/node/interfaces.cpp (18)
hash(853-870)hash(853-853)hash(892-896)hash(892-892)hash(918-923)hash(918-918)tx(609-612)tx(609-609)tx(732-735)tx(732-732)tx(736-739)tx(736-736)tx(757-760)tx(757-757)tx(1002-1009)tx(1002-1002)tx(1021-1036)tx(1021-1021)
src/net_processing.h (6)
src/net_processing.cpp (86)
PeerMisbehaving(6527-6530)PeerMisbehaving(6527-6527)pnode(637-637)pnode(642-642)pnode(652-652)pnode(737-737)pnode(781-781)pnode(853-853)pnode(855-855)PeerIsBanned(6532-6535)PeerIsBanned(6532-6532)node_id(653-653)node_id(748-748)PeerEraseObjectRequest(6537-6540)PeerEraseObjectRequest(6537-6537)nodeid(627-627)nodeid(630-630)nodeid(643-643)nodeid(654-654)nodeid(660-660)nodeid(677-677)nodeid(679-679)nodeid(717-718)nodeid(728-728)nodeid(860-860)nodeid(862-862)nodeid(1038-1038)nodeid(1072-1072)inv(631-631)inv(632-632)inv(655-655)inv(656-656)inv(657-657)inv(669-669)inv(675-675)inv(963-963)inv(2388-2388)PeerRelayInv(6542-6545)PeerRelayInv(6542-6542)PeerRelayInvFiltered(6547-6550)PeerRelayInvFiltered(6547-6547)PeerRelayInvFiltered(6552-6555)PeerRelayInvFiltered(6552-6552)PeerRelayRecoveredSig(6572-6575)PeerRelayRecoveredSig(6572-6572)sig_hash(658-658)PeerAskPeersForTransaction(6557-6560)PeerAskPeersForTransaction(6557-6557)txid(633-633)txid(659-659)txid(663-663)txid(666-666)PeerGetRequestedObjectCount(6562-6565)PeerGetRequestedObjectCount(6562-6562)PeerPostProcessMessage(6567-6570)PeerPostProcessMessage(6567-6567)ret(661-661)ret(683-683)scheduler(623-623)scheduler(649-649)scheduler(692-692)ProcessMessage(3669-5458)ProcessMessage(3669-3674)pfrom(617-617)pfrom(638-639)pfrom(751-753)pfrom(762-762)pfrom(770-770)pfrom(773-773)pfrom(775-775)pfrom(777-777)pfrom(873-873)pfrom(1055-1055)AddExtraHandler(1651-1658)AddExtraHandler(1651-1651)handler(644-644)RemoveHandlers(1660-1665)RemoveHandlers(1660-1660)StartHandlers(1667-1672)StartHandlers(1667-1667)StopHandlers(1674-1682)StopHandlers(1674-1674)InterruptHandlers(1684-1689)InterruptHandlers(1684-1684)ScheduleHandlers(1691-1696)ScheduleHandlers(1691-1691)src/net_instantsend.cpp (7)
inv(186-186)Stop(77-91)Stop(77-77)Start(62-75)Start(62-62)ProcessMessage(17-60)ProcessMessage(17-17)src/init.cpp (3)
Interrupt(230-256)Interrupt(230-230)handler(480-480)src/net_signing.cpp (6)
Stop(156-169)Stop(156-156)Start(144-154)Start(144-144)ProcessMessage(53-142)ProcessMessage(53-53)src/net_signing.h (1)
Start(34-36)src/net_governance.cpp (2)
Schedule(21-54)Schedule(21-21)
src/node/sync.cpp (4)
src/net.h (1)
AllNodes(1297-1297)src/net_processing.cpp (11)
ProcessMessage(3669-5458)ProcessMessage(3669-3674)peer(697-697)peer(832-832)peer(937-937)peer(938-938)peer(1045-1045)peer(1053-1053)pindexNew(608-608)UpdatedBlockTip(2192-2227)UpdatedBlockTip(2192-2192)src/governance/governance.cpp (4)
ProcessMessage(183-325)ProcessMessage(183-184)UpdatedBlockTip(1229-1251)UpdatedBlockTip(1229-1229)src/dsnotificationinterface.cpp (6)
AcceptedBlockHeader(46-50)AcceptedBlockHeader(46-46)NotifyHeaderTip(52-55)NotifyHeaderTip(52-52)UpdatedBlockTip(65-85)UpdatedBlockTip(65-65)
src/net_processing.cpp (2)
src/validationinterface.cpp (4)
RegisterValidationInterface(133-138)RegisterValidationInterface(133-133)UnregisterValidationInterface(145-150)UnregisterValidationInterface(145-145)src/llmq/dkgsessionhandler.cpp (2)
Misbehaving(112-116)Misbehaving(112-112)
src/net_instantsend.cpp (2)
src/net_processing.cpp (24)
ProcessMessage(3669-5458)ProcessMessage(3669-3674)pfrom(617-617)pfrom(638-639)pfrom(751-753)pfrom(762-762)pfrom(770-770)pfrom(773-773)pfrom(775-775)pfrom(777-777)pfrom(873-873)pfrom(1055-1055)hash(1026-1026)hash(1032-1032)WITH_LOCK(331-334)WITH_LOCK(3234-3234)WITH_LOCK(3256-3256)id(701-701)id(705-705)LOCK(323-329)LOCK(336-340)LOCK(341-345)ret(661-661)ret(683-683)src/net_instantsend.h (1)
Start(32-34)
src/net_governance.h (4)
src/governance/governance.cpp (2)
CGovernanceManager(70-86)CGovernanceManager(88-92)src/node/sync.cpp (1)
CMasternodeSync(14-20)src/net_processing.h (3)
NetHandler(74-90)NetHandler(78-81)PeerManagerInternal(58-72)src/net_governance.cpp (8)
Schedule(21-54)Schedule(21-21)ProcessTick(143-323)ProcessTick(143-143)SendGovernanceSyncRequest(56-61)SendGovernanceSyncRequest(56-56)RequestGovernanceObjectVotes(63-141)RequestGovernanceObjectVotes(63-63)
src/llmq/signing_shares.cpp (3)
src/net_processing.cpp (14)
node_id(653-653)node_id(748-748)LOCK(323-329)LOCK(336-340)LOCK(341-345)id(701-701)id(705-705)pnode(637-637)pnode(642-642)pnode(652-652)pnode(737-737)pnode(781-781)pnode(853-853)pnode(855-855)src/spork.h (1)
cs(226-339)src/llmq/signing.cpp (2)
IsQuorumActive(588-597)IsQuorumActive(588-588)
src/llmq/signing.h (4)
src/llmq/signhash.h (3)
llmq(17-52)SignHash(24-46)hash(64-73)src/llmq/signhash.cpp (1)
SignHash(14-22)src/llmq/signing.cpp (17)
CSigningManager(328-332)AlreadyHave(334-347)AlreadyHave(334-334)GetRecoveredSigForGetData(349-359)GetRecoveredSigForGetData(349-349)ProcessRecoveredSig(361-381)ProcessRecoveredSig(361-361)ProcessRecoveredSig(455-494)ProcessRecoveredSig(455-455)FetchPendingReconstructed(447-452)FetchPendingReconstructed(447-447)CollectPendingRecoveredSigsToVerify(383-445)CollectPendingRecoveredSigsToVerify(383-386)GetListeners(496-500)GetListeners(496-496)Cleanup(513-526)Cleanup(513-513)src/net_signing.cpp (2)
ProcessRecoveredSig(171-182)ProcessRecoveredSig(171-171)
src/instantsend/instantsend.h (4)
src/net_instantsend.h (1)
instantsend(12-16)src/instantsend/lock.h (1)
instantsend(18-41)src/instantsend/instantsend.cpp (14)
CInstantSendManager(51-62)CInstantSendManager(64-64)GetCycleBlockHeight(71-78)GetCycleBlockHeight(71-71)IsKnownInstantSend(80-87)IsKnownInstantSend(80-80)EnqueueInstantSendLock(89-110)EnqueueInstantSendLock(89-89)PrepareTxToRetry(391-407)PrepareTxToRetry(391-391)GetPendingLocks(112-143)GetPendingLocks(112-112)ProcessInstantSendLock(145-219)ProcessInstantSendLock(145-146)src/llmq/signing.cpp (1)
CSigningManager(328-332)
src/governance/governance.h (2)
src/governance/governance.cpp (14)
PrepareVotesToRequest(1056-1096)PrepareVotesToRequest(1056-1056)RequestOrphanObjects(1253-1278)RequestOrphanObjects(1253-1253)CleanOrphanObjects(1280-1296)CleanOrphanObjects(1280-1280)CheckAndRemove(415-534)CheckAndRemove(415-415)GetMNManager(536-539)GetMNManager(536-536)FetchRelayInventory(541-547)FetchRelayInventory(541-541)RequestGovernanceObject(1020-1049)RequestGovernanceObject(1020-1020)src/protocol.h (1)
CInv(531-568)
src/llmq/signing_shares.h (3)
src/llmq/signing_shares.cpp (40)
CSigSharesManager(179-189)CSigSharesManager(191-191)CreateSigShare(1348-1416)CreateSigShare(1348-1348)ForceReAnnouncement(1419-1443)ForceReAnnouncement(1419-1419)HandleNewRecoveredSig(1445-1450)HandleNewRecoveredSig(1445-1445)AsyncSignIfMember(642-722)AsyncSignIfMember(642-644)FetchPendingSigShares(1319-1324)FetchPendingSigShares(1319-1319)SignPendingSigShare(1326-1346)SignPendingSigShare(1326-1326)SendMessages(973-1000)SendMessages(973-973)Cleanup(1133-1253)Cleanup(1133-1133)MarkAsBanned(1299-1317)MarkAsBanned(1299-1299)RemoveAsBanned(1282-1297)RemoveAsBanned(1282-1282)GetAllNodes(1270-1280)GetAllNodes(1270-1270)ProcessMessageSigShare(334-379)ProcessMessageSigShare(334-334)ProcessSigShare(495-546)ProcessSigShare(495-495)ProcessMessageSigSesAnn(203-233)ProcessMessageSigSesAnn(203-203)ProcessMessageSigShares(235-278)ProcessMessageSigShares(235-235)ProcessMessageBatchedSigShares(280-332)ProcessMessageBatchedSigShares(280-280)CollectPendingSigSharesToVerify(423-492)CollectPendingSigSharesToVerify(423-425)PreVerifyBatchedSigShares(381-421)PreVerifyBatchedSigShares(381-382)TryRecoverSig(548-625)TryRecoverSig(548-548)src/net_signing.cpp (2)
ProcessMessage(53-142)ProcessMessage(53-53)src/llmq/signhash.h (1)
llmq(17-52)
🪛 GitHub Actions: Check Merge Fast-Forward Only
src/governance/governance.cpp
[error] 1-1: Merge conflict in src/governance/governance.cpp during automatic merge.
src/governance/governance.h
[error] 1-1: Merge conflict in src/governance/governance.h during automatic merge.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
🔇 Additional comments (43)
src/net.h (1)
38-38: Verify thatllmq/params.hprovides all necessary symbols previously available fromconsensus/params.h.This single-line include swap is part of the broader decoupling refactor. Since this is an architectural change affecting the dependency graph, ensure the substituted header exports all type definitions and macros that
net.hand its consumers depend on.If you see build failures mentioning undefined references or missing type definitions (e.g.,
Consensus::types), this change may need adjustment or additional includes to bridge the transition.test/lint/lint-circular-dependencies.py (3)
31-31: New circular dependency introduced in refactoring.This circular dependency chain doesn't involve
net_processing, which aligns with the PR's goal of decoupling network code from consensus components. As a proof-of-concept, this appears acceptable.
55-55: Acceptable circular dependency from sync relocation.This simple circular dependency reflects the relocation of synchronization logic from
masternode/sync.htonode/sync.h. The bidirectional dependency betweennetandnode/syncis expected and architecturally sound for synchronization infrastructure.
32-32: Verify this circular dependency chain against the PR baseline before merging.I was unable to fully verify the original concern due to environment constraints, but I can confirm that line 32 contains a notably long 7-component circular dependency chain with
net_processing. In the current state, there are 4 circular dependencies involvingnet_processing(lines 32, 33, 34, and 52), with line 32 being the longest by far.Before accepting this PR, please manually verify:
- What was the baseline count on the
developbranch? (The PR claims to reduce from 56 to 37 net_processing-related circular dependencies)- Were there longer chains on develop that have since been broken, making line 32 an acceptable remainder?
- Is the 7-component chain the shortest achievable given current module architecture?
src/masternode/active/context.cpp (4)
29-30: I need to directly examine the codebase to verify the architectural claims made in the review comment.Based on the verification performed, I can see that CSigSharesManager still retains ProcessMessage methods for handling sig shares network messages. However, I couldn't fully verify the specific architectural changes regarding the peerman parameter removal and worker thread lifecycle changes due to search limitations. Since the original review comment makes specific architectural claims that require precise confirmation of the constructor signature and thread management changes, and given that the proof-of-concept nature of this PR involves intentional architectural shifts, I'll generate a rewritten review that reflects what can be confirmed and flags what needs verification.
Verify CSigSharesManager architecture properly integrates with net_processing layer after peerman dependency removal.
The removal of the peerman parameter from CSigSharesManager's constructor and the elimination of worker thread management from Start/Stop methods represent significant architectural changes. While CSigSharesManager retains ProcessMessage* methods for handling sig shares messages, confirm that:
- Messages are properly routed from net_processing to CSigSharesManager without direct peer manager access
- Thread safety is maintained for message processing without explicit worker thread lifecycle management
- The NetHandler bridge pattern properly replaces the removed peer manager dependencies
59-63: No changes needed - resource cleanup architecture is correct.The removal of explicit worker thread stopping is appropriate because CSigSharesManager has a default destructor and shareman is managed as a unique_ptr in ActiveContext. Worker threads belong to NetSigning (the NetHandler), not to CSigSharesManager itself. The call to
UnregisterAsRecoveredSigsListener()correctly prevents new callbacks before destruction, and the unique_ptr ensures automatic cleanup. The threading lifecycle is properly managed at the NetSigning layer, not at the ActiveContext layer.
49-51: The empty Interrupt() method is intentional and correct.ActiveContext manages lifecycle through Start/Stop methods only. Start() initiates qdkgsman threads and listener registration; Stop() unregisters listeners and stops threads. No managed entity (shareman, gov_signer, cj_server, ehf_sighandler) exposes an Interrupt() method. LLMQContext follows the identical pattern with an empty Interrupt(). The architecture does not use thread-based async operations requiring separate interruption signals—thread management is handled directly in Start/Stop, and listener lifecycle through registration/unregistration.
The review comment's assumptions about NetHandler-based processing and worker thread removal in shareman do not match the actual implementation.
Likely an incorrect or invalid review comment.
53-57: NetHandler properly replaces shareman worker thread lifecycle—verified correct.The architectural refactoring is sound. Worker threads are now managed by NetSigning (a NetHandler subclass) rather than shareman directly. Initialization sequence (init.cpp:2283–2285) ensures threads start via
peerman->StartHandlers()beforeactive_ctx->Start()registers the listener. Thread safety is maintained with existing RecursiveMutex and Mutex guards. Null checks protect non-masternode scenarios.src/node/context.cpp (1)
23-23: Include swap LGTMnode/sync.h import here is consistent with the refactor; no functional impact in this TU.
src/coinjoin/coinjoin.cpp (2)
19-19: Include swap LGTMSwitching to node/sync.h aligns with the decoupling goal.
57-61: No API mismatch; review comment is incorrectThe header declaration (
src/coinjoin/coinjoin.h:215) provides a default argument:bool IsTimeOutOfBounds(int64_t current_time = GetAdjustedTime()) const;All call sites—including those in
coinjoin.cpp:139,coinjoin.cpp:155,server.cpp:161, andclient.cpp:94—invoke the method without arguments and will compile successfully using the defaultGetAdjustedTime(). The implementation incoinjoin.cpp:57correctly omits repeating the default argument (standard C++ practice).Likely an incorrect or invalid review comment.
src/evo/mnauth.cpp (1)
13-13: Include swap LGTMnode/sync.h provides CMasternodeSync used here; no behavior change.
Please confirm node/sync.h exports the same PROTECTED API surface required by CMNAuth::ProcessMessage to keep linkage stable.
src/masternode/payments.cpp (1)
15-15: Include swap LGTMNo functional change; payments logic unaffected.
src/governance/vote.cpp (1)
10-10: Include swap LGTMGovernance voting compiles against node/sync.h; no behavior change expected.
src/coinjoin/server.cpp (1)
11-11: Include swap LGTMServer side uses CMasternodeSync; node/sync.h import is consistent across the refactor.
src/net.cpp (1)
44-44: Refactor verified - stale includes removedVerification confirms the refactor is complete. The old
<masternode/sync.h>include has been fully removed from the codebase, and<node/sync.h>correctly providesCMasternodeSync. The change to src/net.cpp is properly aligned with the refactor.src/masternode/utils.cpp (1)
13-13: Review comment is incorrect—symbols are properly exported from their respective headersThe review comment incorrectly assumes
PROBE_WAIT_INTERVALis exported bynode/sync.h. In reality,PROBE_WAIT_INTERVALis defined innet.h(line 73), which the file already includes directly (line 7). The#include <node/sync.h>is forCMasternodeSync, which is properly exported from that header. The code correctly accesses both symbols from their appropriate sources with no missing includes or risk of silent behavior drift.Likely an incorrect or invalid review comment.
src/governance/signing.cpp (1)
10-10: LGTM: Header relocation is consistent with the PR's architectural changes.The migration from
masternode/sync.htonode/sync.haligns with the broader refactor to consolidate synchronization logic at the node level.src/chainlock/chainlock.cpp (1)
21-21: LGTM: Consistent header migration.src/coinjoin/client.cpp (1)
13-13: LGTM: Header relocation follows the established pattern.src/governance/object.cpp (1)
12-12: LGTM: Header change is consistent.src/chainlock/signing.cpp (2)
13-13: LGTM: Header relocation is consistent.
144-144: AsyncSignIfMember refactoring verified as complete.All call sites in the codebase have been successfully updated to use the new signature without the
m_sigmanparameter:
- 5 call sites confirmed across:
src/rpc/quorums.cpp,src/llmq/ehf_signals.cpp,src/instantsend/signing.cpp(2 calls), andsrc/chainlock/signing.cpp- CSigSharesManager properly manages signing through its
CSigningManager& sigmanmember variable rather than accepting it as a parameter- No remaining calls with the old signature pattern were found
The decoupling is complete and correctly implemented.
src/test/util/setup_common.cpp (2)
20-20: LGTM: Header additions prepare for future net handler integration.The new includes for
masternode/active/context.h,net_instantsend.h, andnet_signing.halign with the introduction of NetHandler components mentioned in the PR objectives.Also applies to: 23-24, 70-70
370-375: Note: Commented-out net handler integration.The commented code indicates future integration of NetInstantSend, NetSigning, and NetGovernance handlers in the test setup. The TODO comment suggests uncertainty about whether these handlers are needed in regression tests.
This is appropriate for a proof-of-concept PR that will be split into smaller, focused changes.
src/llmq/ehf_signals.cpp (2)
81-81: AsyncSignIfMember signature updated with additional parameters.The call includes
quorumHashand boolean flags not present in the chainlock/signing.cpp call site. This suggests different overloads or a more complete signature for EHF signing.
114-127: LGTM: HandleNewRecoveredSig refactored to early-return pattern.The refactoring is functionally equivalent:
- Success: immediately return
MessageProcessingResultwith transaction hash- Failure: immediately return empty result after logging
- No match: fall through to return empty result
The tighter lock scope around
ProcessTransactionis a good practice.src/Makefile.am (2)
298-300: LGTM: Build configuration updated to include new net handler headers.The addition of
net_governance.h,net_instantsend.h,net_signing.h, andnode/sync.hproperly registers the new components introduced in this refactor.Also applies to: 318-318
562-564: LGTM: Source files correctly added to the node library.The new net handler implementations and relocated sync source are properly included in
libbitcoin_node_a_SOURCES.Also applies to: 577-577
src/instantsend/instantsend.h (6)
8-10: LGTM!The added includes are appropriate for the header's dependencies on InstantSend-specific types and base classes.
22-23: LGTM!The
<variant>include is needed forProcessInstantSendLock's return type, and<vector>is used byPrepareTxToRetry.
40-40: LGTM!The updated
PendingStatestructure appropriately tracks pending InstantSend locks with their source peer, supporting the new NetInstantSend integration.
100-100: LGTM!The
Signer()accessor appropriately exposes the internal signer for NetInstantSend integration. Callers correctly handle the nullable pointer, as seen in theProcessInstantSendLockimplementation.
125-138: Well-designed interface for NetInstantSend integration.The new public helpers appropriately expose the minimal interface needed for network layer communication while maintaining proper encapsulation. The methods include appropriate lock annotations (except for the issue flagged separately), and the use of
std::variantforProcessInstantSendLock's return type clearly communicates the three possible outcomes (unknown/ignored, transaction found, transaction not found).
87-89: Constructor signature changes verified and correctly implemented.All call sites have been updated. The only instantiation of
CInstantSendManageratsrc/llmq/context.cpp:35-36correctly passes all parameters matching the new signature. Themn_syncparameter is properlyconstat the call site, and parameter name differences (unit_testsvsunitTests,wipevsfWipe) are inconsequential—the types align correctly.src/net_processing.cpp (3)
60-60: LGTM: include updatesUsing llmq/signhash.h and node/sync.h here is appropriate given the new PlatformBan verification and sync header move.
Also applies to: 64-64
644-650: Handler management API addition looks goodThe Add/Remove/Start/Stop/Interrupt/Schedule surface aligns with the bridge intent.
Please confirm these are invoked from init/shutdown in the intended order (Add → Start/Schedule; Interrupt → Stop → Remove) for all build configs.
651-662: PeerManagerInternal wrappers are fineThin wrappers correctly delegate to existing internals.
src/llmq/signing_shares.h (5)
360-366: LGTM! Well-designed data transfer object.The
PendingSignatureDatastruct appropriately encapsulates the parameters needed for deferred signature share creation. The const members prevent accidental modification, and the constructor properly moves the shared_ptr quorum for efficiency.
369-372: Helpful markers for future refactoring.These TODO comments appropriately document the architectural direction, acknowledging that the current refactoring is an intermediate step toward further decoupling. This aligns well with the PR's proof-of-concept nature and helps reviewers understand the intended evolution.
467-468: Excellent refactoring to static method.Converting
PreVerifyBatchedSigSharesto a static method with explicit parameters is good practice. This makes dependencies explicit, enables easier testing, and demonstrates that the method doesn't require instance state.
444-444: Appropriate use of [[nodiscard]].The
[[nodiscard]]attribute onSignPendingSigSharecorrectly ensures callers check whether signature recovery succeeded. Since the method may returnnullptron failure, ignoring the return value would lose critical information about the operation's success.
417-419: Successfully removes PeerManager dependency.The updated constructor signature successfully eliminates the
PeerManagerdependency, which is a primary goal of this refactoring. The remaining dependencies are appropriate for the class's functionality and support the new NetSigning delegation pattern.
|
|
||
| [[nodiscard]] MessageProcessingResult ProcessMessage(NodeId from, std::string_view msg_type, CDataStream& vRecv) | ||
| /* Helpers for communications between CInstantSendManager & NetInstantSend */ | ||
| // GetCycleBlockHeight returns negative number if cycle_hash is now known |
There was a problem hiding this comment.
Fix typo in comment.
The comment says "if cycle_hash is now known" but should say "if cycle_hash is not known" (based on the method returning a negative number to indicate unknown hash).
Apply this diff:
- // GetCycleBlockHeight returns negative number if cycle_hash is now known
+ // GetCycleBlockHeight returns negative number if cycle_hash is not known📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // GetCycleBlockHeight returns negative number if cycle_hash is now known | |
| // GetCycleBlockHeight returns negative number if cycle_hash is not known |
🤖 Prompt for AI Agents
In src/instantsend/instantsend.h at line 126 the comment reads
"GetCycleBlockHeight returns negative number if cycle_hash is now known" —
change the typo "now" to "not" so it reads "GetCycleBlockHeight returns negative
number if cycle_hash is not known".
| bool IsKnownInstantSend(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks); | ||
| void EnqueueInstantSendLock(NodeId from, const uint256& hash, const std::shared_ptr<instantsend::InstantSendLock>& islock) | ||
| EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks); | ||
| std::vector<CTransactionRef> PrepareTxToRetry() EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, cs_pendingRetry); |
There was a problem hiding this comment.
Fix incorrect lock annotation.
The annotation states cs_pendingRetry must already be held, but the implementation uses LOCK2(cs_nonLocked, cs_pendingRetry), which acquires both locks. This inconsistency can mislead about thread safety guarantees and lock ordering.
Apply this diff:
- std::vector<CTransactionRef> PrepareTxToRetry() EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, cs_pendingRetry);
+ std::vector<CTransactionRef> PrepareTxToRetry() EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingRetry);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::vector<CTransactionRef> PrepareTxToRetry() EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, cs_pendingRetry); | |
| std::vector<CTransactionRef> PrepareTxToRetry() EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingRetry); |
🤖 Prompt for AI Agents
In src/instantsend/instantsend.h around line 131, the lock annotation is
incorrect (it currently reads EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked,
cs_pendingRetry) while the implementation acquires both locks with
LOCK2(cs_nonLocked, cs_pendingRetry)); remove the stray '!' and update the
annotation to require both locks (e.g., EXCLUSIVE_LOCKS_REQUIRED(cs_nonLocked,
cs_pendingRetry)) so the declaration matches the implementation and correctly
documents the required exclusive locks.
| vector<NodeId> nodes; | ||
|
|
||
| LOCK(cs); | ||
| for (auto it = nodeStates.begin(); it != nodeStates.end();) { | ||
| if (m_peerman.IsBanned(it->first)) { | ||
| // re-request sigshares from other nodes | ||
| // TODO: remove NO_THREAD_SAFETY_ANALYSIS | ||
| // using here template ForEach makes impossible to use lock annotation | ||
| it->second.requestedSigShares.ForEach([this](const SigShareKey& k, int64_t) NO_THREAD_SAFETY_ANALYSIS { | ||
| AssertLockHeld(cs); | ||
| sigSharesRequested.Erase(k); | ||
| }); | ||
| it = nodeStates.erase(it); | ||
| } else { | ||
| ++it; | ||
| } | ||
| nodes.reserve(nodeStates.size()); | ||
| for (const auto& [node_id, _] : nodeStates) { | ||
| nodes.push_back(node_id); | ||
| } |
There was a problem hiding this comment.
Fix missing namespace on vector.
vector<NodeId> lacks the std:: qualifier, which breaks compilation. Please qualify it so it builds again.
- vector<NodeId> nodes;
+ std::vector<NodeId> nodes;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vector<NodeId> nodes; | |
| LOCK(cs); | |
| for (auto it = nodeStates.begin(); it != nodeStates.end();) { | |
| if (m_peerman.IsBanned(it->first)) { | |
| // re-request sigshares from other nodes | |
| // TODO: remove NO_THREAD_SAFETY_ANALYSIS | |
| // using here template ForEach makes impossible to use lock annotation | |
| it->second.requestedSigShares.ForEach([this](const SigShareKey& k, int64_t) NO_THREAD_SAFETY_ANALYSIS { | |
| AssertLockHeld(cs); | |
| sigSharesRequested.Erase(k); | |
| }); | |
| it = nodeStates.erase(it); | |
| } else { | |
| ++it; | |
| } | |
| nodes.reserve(nodeStates.size()); | |
| for (const auto& [node_id, _] : nodeStates) { | |
| nodes.push_back(node_id); | |
| } | |
| std::vector<NodeId> nodes; | |
| LOCK(cs); | |
| nodes.reserve(nodeStates.size()); | |
| for (const auto& [node_id, _] : nodeStates) { | |
| nodes.push_back(node_id); | |
| } |
🤖 Prompt for AI Agents
In src/llmq/signing_shares.cpp around lines 1272 to 1278, the declaration uses
unqualified vector<NodeId> which causes a compilation error; change it to
std::vector<NodeId> and ensure <vector> is included or already present in the
file, then rebuild to confirm the missing namespace is resolved.
| std::vector<CRecoveredSigsListener*> CSigningManager::GetListeners() const | ||
| { | ||
| LOCK(cs_listeners); | ||
| return recoveredSigsListeners; | ||
| } |
There was a problem hiding this comment.
Guard recovered-sig listeners against post-unregister use
GetListeners() now hands out raw CRecoveredSigsListener* without holding cs_listeners while they’re consumed. In the new flow (NetSigning::ProcessRecoveredSig), we copy the vector and release the mutex before iterating, so a concurrent UnregisterRecoveredSigsListener() (e.g. during shutdown) can erase the entry and destroy the object while another thread still dereferences the stale pointer. That’s a latent use-after-free that didn’t exist when callbacks were invoked under the lock. Please keep the listeners’ lifetime protected—either continue invoking callbacks while cs_listeners is held, or promote the stored handles to something that keeps the target alive (e.g. shared_ptr/weak_ptr). As written this is a critical concurrency regression.
| scheduler.scheduleEvery( | ||
| [this, &connman]() -> void { | ||
| if (ShutdownRequested()) return; | ||
| ProcessTick(connman); | ||
| }, | ||
| std::chrono::seconds{1}); | ||
|
|
||
| scheduler.scheduleEvery( | ||
| [this, &connman]() -> void { | ||
| if (!m_node_sync.IsSynced()) return; | ||
|
|
||
| // CHECK OBJECTS WE'VE ASKED FOR, REMOVE OLD ENTRIES | ||
| m_gov_manager.CleanOrphanObjects(); | ||
| m_gov_manager.RequestOrphanObjects(connman); | ||
|
|
||
| // CHECK AND REMOVE - REPROCESS GOVERNANCE OBJECTS | ||
| m_gov_manager.CheckAndRemove(); | ||
| }, | ||
| std::chrono::minutes{5}); | ||
|
|
||
| scheduler.scheduleEvery( | ||
| [this]() -> void { | ||
| auto relay_invs = m_gov_manager.FetchRelayInventory(); | ||
| for (const auto& inv : relay_invs) { | ||
| m_peer_manager->PeerRelayInv(inv); | ||
| } | ||
| }, | ||
| // Tests need tighter timings to avoid timeouts, use more relaxed pacing otherwise | ||
| Params().IsMockableChain() ? std::chrono::seconds{1} : std::chrono::seconds{5}); | ||
| } |
There was a problem hiding this comment.
Cancel scheduled governance callbacks before destruction.
PeerManagerImpl::RemoveHandlers() clears m_handlers while the scheduler thread keeps running. The three scheduleEvery lambdas here capture this, so after the handler is erased the scheduler continues to invoke them with a dangling pointer, leading to use-after-free crashes during shutdown (and in any future hot-handler reload). We need to keep the returned FunctionHandles and cancel them from an overridden Stop() prior to destruction (and teach the header about the new members).
@@
- scheduler.scheduleEvery(
+ m_tick_handle = scheduler.scheduleEvery(
@@
- scheduler.scheduleEvery(
+ m_maintenance_handle = scheduler.scheduleEvery(
@@
- scheduler.scheduleEvery(
+ m_relay_handle = scheduler.scheduleEvery(
[this]() -> void {
auto relay_invs = m_gov_manager.FetchRelayInventory();
for (const auto& inv : relay_invs) {
m_peer_manager->PeerRelayInv(inv);
}
},
Params().IsMockableChain() ? std::chrono::seconds{1} : std::chrono::seconds{5});
}
+
+void NetGovernance::Stop()
+{
+ m_tick_handle.reset();
+ m_maintenance_handle.reset();
+ m_relay_handle.reset();
+}(Declare the three CScheduler::FunctionHandle members and override Stop() in net_governance.h accordingly.)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/net_governance.cpp around lines 25-54, the three scheduler.scheduleEvery
lambdas capture this and can run after handlers are removed, causing
use-after-free; store the CScheduler::FunctionHandle returned from each
scheduleEvery into three new member variables declared in net_governance.h,
override Stop() in the class to call cancel() (or the scheduler's equivalent) on
those FunctionHandles before handler removal/destruction, and update the header
to add the three FunctionHandle members plus the Stop() override so the
scheduled callbacks are cancelled prior to destruction.
| #include <net_processing.h> | ||
|
|
||
| #include <util/threadinterrupt.h> | ||
|
|
||
| namespace instantsend | ||
| { | ||
| struct InstantSendLock; | ||
| using InstantSendLockPtr = std::shared_ptr<InstantSendLock>; | ||
| } // namespace instantsend | ||
| namespace llmq { | ||
| class CInstantSendManager; | ||
| class CQuorumManager; | ||
| } // namespace llmq | ||
|
|
||
| class NetInstantSend final : public NetHandler | ||
| { | ||
| public: | ||
| NetInstantSend(PeerManagerInternal* peer_manager, llmq::CInstantSendManager& is_manager, llmq::CQuorumManager& qman) | ||
| : NetHandler(peer_manager), m_is_manager{is_manager}, m_qman(qman) | ||
| { | ||
| workInterrupt.reset(); | ||
| } | ||
| void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv) override; | ||
|
|
||
| void Start() override; | ||
| void Stop() override; | ||
| void Interrupt() override { workInterrupt(); }; | ||
|
|
||
| void WorkThreadMain(); | ||
| private: | ||
| void ProcessPendingISLocks(Uint256HashMap<std::pair<NodeId, instantsend::InstantSendLockPtr>>&& locks_to_process); | ||
|
|
||
| Uint256HashSet ProcessPendingInstantSendLocks(const Consensus::LLMQParams& llmq_params, int signOffset, bool ban, | ||
| const Uint256HashMap<std::pair<NodeId, instantsend::InstantSendLockPtr>>& pend); | ||
| // EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry); | ||
| llmq::CInstantSendManager& m_is_manager; | ||
| llmq::CQuorumManager& m_qman; | ||
|
|
||
| std::thread workThread; | ||
| CThreadInterrupt workInterrupt; |
There was a problem hiding this comment.
Include <thread> before declaring std::thread
This header declares std::thread workThread but never includes <thread>, so any TU that includes net_instantsend.h before pulling in <thread> will fail to compile. Please add the missing include.
#include <net_processing.h>
#include <util/threadinterrupt.h>
+
+#include <thread>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <net_processing.h> | |
| #include <util/threadinterrupt.h> | |
| namespace instantsend | |
| { | |
| struct InstantSendLock; | |
| using InstantSendLockPtr = std::shared_ptr<InstantSendLock>; | |
| } // namespace instantsend | |
| namespace llmq { | |
| class CInstantSendManager; | |
| class CQuorumManager; | |
| } // namespace llmq | |
| class NetInstantSend final : public NetHandler | |
| { | |
| public: | |
| NetInstantSend(PeerManagerInternal* peer_manager, llmq::CInstantSendManager& is_manager, llmq::CQuorumManager& qman) | |
| : NetHandler(peer_manager), m_is_manager{is_manager}, m_qman(qman) | |
| { | |
| workInterrupt.reset(); | |
| } | |
| void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv) override; | |
| void Start() override; | |
| void Stop() override; | |
| void Interrupt() override { workInterrupt(); }; | |
| void WorkThreadMain(); | |
| private: | |
| void ProcessPendingISLocks(Uint256HashMap<std::pair<NodeId, instantsend::InstantSendLockPtr>>&& locks_to_process); | |
| Uint256HashSet ProcessPendingInstantSendLocks(const Consensus::LLMQParams& llmq_params, int signOffset, bool ban, | |
| const Uint256HashMap<std::pair<NodeId, instantsend::InstantSendLockPtr>>& pend); | |
| // EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry); | |
| llmq::CInstantSendManager& m_is_manager; | |
| llmq::CQuorumManager& m_qman; | |
| std::thread workThread; | |
| CThreadInterrupt workInterrupt; | |
| #include <net_processing.h> | |
| #include <util/threadinterrupt.h> | |
| #include <thread> | |
| namespace instantsend | |
| { | |
| struct InstantSendLock; | |
| using InstantSendLockPtr = std::shared_ptr<InstantSendLock>; | |
| } // namespace instantsend | |
| namespace llmq { | |
| class CInstantSendManager; | |
| class CQuorumManager; | |
| } // namespace llmq | |
| class NetInstantSend final : public NetHandler | |
| { | |
| public: | |
| NetInstantSend(PeerManagerInternal* peer_manager, llmq::CInstantSendManager& is_manager, llmq::CQuorumManager& qman) | |
| : NetHandler(peer_manager), m_is_manager{is_manager}, m_qman(qman) | |
| { | |
| workInterrupt.reset(); | |
| } | |
| void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv) override; | |
| void Start() override; | |
| void Stop() override; | |
| void Interrupt() override { workInterrupt(); }; | |
| void WorkThreadMain(); | |
| private: | |
| void ProcessPendingISLocks(Uint256HashMap<std::pair<NodeId, instantsend::InstantSendLockPtr>>&& locks_to_process); | |
| Uint256HashSet ProcessPendingInstantSendLocks(const Consensus::LLMQParams& llmq_params, int signOffset, bool ban, | |
| const Uint256HashMap<std::pair<NodeId, instantsend::InstantSendLockPtr>>& pend); | |
| // EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry); | |
| llmq::CInstantSendManager& m_is_manager; | |
| llmq::CQuorumManager& m_qman; | |
| std::thread workThread; | |
| CThreadInterrupt workInterrupt; |
🤖 Prompt for AI Agents
In src/net_instantsend.h around lines 8 to 47, the header declares std::thread
workThread but does not include <thread>, which can cause compilation failures
when this header is included before <thread>; add #include <thread> near the
other includes (e.g., after <util/threadinterrupt.h>) so std::thread is defined
before its use.
| ~PeerManagerImpl() { | ||
| // probably do some disassignment | ||
| // m_handlers.clear(); | ||
| } |
There was a problem hiding this comment.
Fix lifecycle: unregister validation callbacks and clear handlers in destructor
Handlers registered as CValidationInterface via AddExtraHandler() must be unregistered before destruction to avoid dangling pointers in g_signals. Currently the destructor does nothing; base NetHandler dtor calling Interrupt/Stop is insufficient because UnregisterValidationInterface(i) is done in StopHandlers(), not in the handler. Call RemoveHandlers() here.
Apply this diff:
- ~PeerManagerImpl() {
- // probably do some disassignment
-// m_handlers.clear();
- }
+ ~PeerManagerImpl() override {
+ // Ensure handlers are interrupted, stopped (which unregisters validation
+ // callbacks), and destroyed before PeerManagerImpl goes away.
+ RemoveHandlers();
+ }Reference: RegisterValidationInterface/UnregisterValidationInterface is managed by the caller, not the callee (see src/validationinterface.cpp). Based on learnings.
🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 598-601, the PeerManagerImpl destructor
currently does nothing; update it to call RemoveHandlers() so that any
CValidationInterface handlers registered via AddExtraHandler() are unregistered
and m_handlers are cleared before destruction. Ensure RemoveHandlers() is
invoked from the destructor path (and only once) to unregister validation
callbacks and clear handler state to avoid dangling pointers in g_signals.
| void PeerManagerImpl::AddExtraHandler(std::unique_ptr<NetHandler>&& handler) | ||
| { | ||
| assert(handler != nullptr); | ||
| if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) { | ||
| RegisterValidationInterface(i); | ||
| } | ||
| m_handlers.emplace_back(std::move(handler)); | ||
| } | ||
|
|
||
| void PeerManagerImpl::RemoveHandlers() | ||
| { | ||
| InterruptHandlers(); | ||
| StopHandlers(); | ||
| m_handlers.clear(); | ||
| } | ||
|
|
||
| void PeerManagerImpl::StartHandlers() | ||
| { | ||
| for (auto& handler : m_handlers) { | ||
| handler->Start(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Balance validation interface registration with guaranteed unregistration
AddExtraHandler() registers CValidationInterface if present; StopHandlers() unregisters it. This is correct, but only if StopHandlers() is always called before destruction. Ensure the destructor fix (calling RemoveHandlers()) lands; otherwise g_signals will retain a dangling pointer.
Linking to the destructor comment above.
🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 1651 to 1672, AddExtraHandler registers a
CValidationInterface from a handler but RemoveHandlers only stops/clears
handlers; ensure registration is balanced by guaranteeing unregistration: either
modify the PeerManagerImpl destructor to call RemoveHandlers() (so
StopHandlers/Unregister happens before destruction) or update RemoveHandlers()
to iterate m_handlers and call UnregisterValidationInterface for any
CValidationInterface before clearing; implement one of these fixes so g_signals
does not retain dangling pointers.
| if (!Params().GetLLMQ(llmqType).has_value()) { | ||
| m_peer_manager->PeerMisbehaving(pfrom.GetId(), 100); | ||
| } | ||
| if (PreVerifyRecoveredSig(llmqType, m_sig_manager.Qman(), *recoveredSig)) { | ||
| m_sig_manager.ProcessRecoveredSig(pfrom.GetId(), std::move(recoveredSig)); | ||
| } |
There was a problem hiding this comment.
Return immediately after flagging an invalid LLMQ type
Once we know llmqType isn’t configured, we call PeerMisbehaving but continue into PreVerifyRecoveredSig. That path ends up in CQuorumManager::GetQuorum, which asserts that the type is valid. As written, a malicious peer can send a bogus llmqType, trigger the assert, and take down the node. Please bail out right after flagging the misbehavior:
auto llmqType = recoveredSig->getLlmqType();
if (!Params().GetLLMQ(llmqType).has_value()) {
m_peer_manager->PeerMisbehaving(pfrom.GetId(), 100);
- }
- if (PreVerifyRecoveredSig(llmqType, m_sig_manager.Qman(), *recoveredSig)) {
+ return;
+ }
+ if (PreVerifyRecoveredSig(llmqType, m_sig_manager.Qman(), *recoveredSig)) {
m_sig_manager.ProcessRecoveredSig(pfrom.GetId(), std::move(recoveredSig));
}🤖 Prompt for AI Agents
In src/net_signing.cpp around lines 63 to 68, after detecting that
Params().GetLLMQ(llmqType) has no value and calling
m_peer_manager->PeerMisbehaving(pfrom.GetId(), 100), stop further processing of
this message immediately (do not fall through into PreVerifyRecoveredSig). Fix
by bailing out right after flagging the misbehavior — return from the handler
(or otherwise skip/continue to the next message) so that PreVerifyRecoveredSig
and subsequent calls into CQuorumManager are not executed with an invalid
llmqType.
| #include <net_processing.h> | ||
| #include <validationinterface.h> | ||
| #include <util/threadinterrupt.h> | ||
|
|
||
| #include <memory> | ||
|
|
||
| namespace llmq { | ||
| class CSigSharesManager; | ||
| class CSigningManager; | ||
| } // namespace llmq | ||
| class NetSigning final : public NetHandler, public CValidationInterface | ||
| { | ||
| public: | ||
| NetSigning(PeerManagerInternal* peer_manager, llmq::CSigningManager& sig_manager, llmq::CSigSharesManager* shares_manager) | ||
| : NetHandler(peer_manager), m_sig_manager(sig_manager), m_shares_manager(shares_manager) | ||
| { | ||
| workInterrupt.reset(); | ||
| } | ||
| void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv) override; | ||
|
|
||
| protected: | ||
| // CValidationInterface | ||
| void NotifyRecoveredSig(const std::shared_ptr<const llmq::CRecoveredSig>& sig) override; | ||
|
|
||
| // NetSigning | ||
| void Start() override; | ||
| void Stop() override; | ||
| void Interrupt() override { workInterrupt(); }; | ||
|
|
||
| void WorkThreadSigning(); | ||
| void WorkThreadShares(); | ||
|
|
||
| private: | ||
| bool ProcessPendingSigShares(); | ||
| bool ProcessPendingRecoveredSigs(); | ||
| void ProcessRecoveredSig(const std::shared_ptr<const llmq::CRecoveredSig>& recoveredSig); | ||
| void BanNode(NodeId id); | ||
| void RemoveBannedNodeStates(); | ||
|
|
||
| private: | ||
| llmq::CSigningManager& m_sig_manager; | ||
| llmq::CSigSharesManager* m_shares_manager; | ||
|
|
||
| std::thread signing_thread; | ||
| std::thread shares_thread; | ||
|
|
||
| CThreadInterrupt workInterrupt; | ||
| }; |
There was a problem hiding this comment.
Add <thread> include for std::thread members
NetSigning defines std::thread signing_thread/shares_thread but the header does not include <thread>, leading to compile errors when this file is included first in a TU. Please include <thread> alongside the other standard headers.
#include <net_processing.h>
#include <validationinterface.h>
#include <util/threadinterrupt.h>
#include <memory>
+#include <thread>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <net_processing.h> | |
| #include <validationinterface.h> | |
| #include <util/threadinterrupt.h> | |
| #include <memory> | |
| namespace llmq { | |
| class CSigSharesManager; | |
| class CSigningManager; | |
| } // namespace llmq | |
| class NetSigning final : public NetHandler, public CValidationInterface | |
| { | |
| public: | |
| NetSigning(PeerManagerInternal* peer_manager, llmq::CSigningManager& sig_manager, llmq::CSigSharesManager* shares_manager) | |
| : NetHandler(peer_manager), m_sig_manager(sig_manager), m_shares_manager(shares_manager) | |
| { | |
| workInterrupt.reset(); | |
| } | |
| void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv) override; | |
| protected: | |
| // CValidationInterface | |
| void NotifyRecoveredSig(const std::shared_ptr<const llmq::CRecoveredSig>& sig) override; | |
| // NetSigning | |
| void Start() override; | |
| void Stop() override; | |
| void Interrupt() override { workInterrupt(); }; | |
| void WorkThreadSigning(); | |
| void WorkThreadShares(); | |
| private: | |
| bool ProcessPendingSigShares(); | |
| bool ProcessPendingRecoveredSigs(); | |
| void ProcessRecoveredSig(const std::shared_ptr<const llmq::CRecoveredSig>& recoveredSig); | |
| void BanNode(NodeId id); | |
| void RemoveBannedNodeStates(); | |
| private: | |
| llmq::CSigningManager& m_sig_manager; | |
| llmq::CSigSharesManager* m_shares_manager; | |
| std::thread signing_thread; | |
| std::thread shares_thread; | |
| CThreadInterrupt workInterrupt; | |
| }; | |
| #include <net_processing.h> | |
| #include <validationinterface.h> | |
| #include <util/threadinterrupt.h> | |
| #include <memory> | |
| #include <thread> | |
| namespace llmq { | |
| class CSigSharesManager; | |
| class CSigningManager; | |
| } // namespace llmq | |
| class NetSigning final : public NetHandler, public CValidationInterface | |
| { | |
| public: | |
| NetSigning(PeerManagerInternal* peer_manager, llmq::CSigningManager& sig_manager, llmq::CSigSharesManager* shares_manager) | |
| : NetHandler(peer_manager), m_sig_manager(sig_manager), m_shares_manager(shares_manager) | |
| { | |
| workInterrupt.reset(); | |
| } | |
| void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv) override; | |
| protected: | |
| // CValidationInterface | |
| void NotifyRecoveredSig(const std::shared_ptr<const llmq::CRecoveredSig>& sig) override; | |
| // NetSigning | |
| void Start() override; | |
| void Stop() override; | |
| void Interrupt() override { workInterrupt(); }; | |
| void WorkThreadSigning(); | |
| void WorkThreadShares(); | |
| private: | |
| bool ProcessPendingSigShares(); | |
| bool ProcessPendingRecoveredSigs(); | |
| void ProcessRecoveredSig(const std::shared_ptr<const llmq::CRecoveredSig>& recoveredSig); | |
| void BanNode(NodeId id); | |
| void RemoveBannedNodeStates(); | |
| private: | |
| llmq::CSigningManager& m_sig_manager; | |
| llmq::CSigSharesManager* m_shares_manager; | |
| std::thread signing_thread; | |
| std::thread shares_thread; | |
| CThreadInterrupt workInterrupt; | |
| }; |
🤖 Prompt for AI Agents
In src/net_signing.h around lines 9 to 56, the header declares std::thread
members (signing_thread, shares_thread) but does not include the <thread>
header; add a #include <thread> alongside the other standard includes (e.g.,
with <memory>) so the std::thread type is available when this header is the
first inclusion unit, then rebuild to verify the missing include error is
resolved.
1 participant
This PR is not meant to get merged, it's a proof-of-concept and consider it as a draft, see #What was done
Issue being fixed or feature implemented
Project of separation of consensus & chain code and network & node is blocked by tight connection of network and consensus code; in components such as llmq::CSigningManager, llmq::CSigSharesManager, coinjoin::client, coinjoin::server, governance, llmq::CInstantSendManager, etc.
What was done?
This PR is not meant to get merged as it is. Currently, it's a proof-of-concept that this approach is legit and could work for complex modules such as llmq::CSigSharesManager and CInstantSendManager.
Once it is done, it will unlock all backports related to kernel project, such as:
bitcoin-chainstatebitcoin/bitcoin#24304libbitcoinkernelbitcoin/bitcoin#24322ChainstateManager::m_adjusted_time_callbackbitcoin/bitcoin#25064GetUTXOStatscodepaths, decouple fromcoinstatsindexbitcoin/bitcoin#24410GetUTXOStatscodepaths, decouple fromcoinstatsindexbitcoin/bitcoin#24410(list is not exhaustive, but illustrative)
Prior approarches with using
tl::expected(#5782 and other PRs),std::optional(such as #6282),MessageProcessingResult(#6292) helped to refactor code and prepare, reduced some dependencies, but they are not sufficient enough to provide reliable solution that works for complex non-trivial usages of PeerManager and network code all over codebase.New approach introduces
NetHandlerwhich is a bridge interface between node/network related code and between consensus/chain code. In case if there is no need any network, objects that inherited by NetHandler are not created.As a positive side-effect, amount of circular dependencies over net_processing is drastically reduced (56 of them to just 37).
How Has This Been Tested?
Run unit & functional tests.
Use linter
test/lint-lint-circular-dependencies.pyExpected some thread safety analysis to fail, maybe some linters won't be happy.
Breaking Changes
N/A
Checklist: